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

Regression with RedundantConditionGivenDocblockType and DocblockTypeContradiction #10715

Closed
ArtemGoutsoul opened this issue Feb 16, 2024 · 11 comments · Fixed by #10756
Closed

Comments

@ArtemGoutsoul
Copy link
Contributor

ArtemGoutsoul commented Feb 16, 2024

Psalm incorrectly reports truthy and falsy values, and marks a redundant check or a doc type contradiction:

This broke in 5.19.1 I think

Consider:

/**
 * @param array<string, true> $array
 */
function func(array $array): int
{
    if (!empty($array['stuff'])) return 1;
    
    if (empty($array['more stuff'])) return 2;

    return 3;
}

Psalm returns:

Psalm output (using commit [c488d40](https://github.com/vimeo/psalm/commit/c488d40)): 

ERROR: [RedundantConditionGivenDocblockType](https://psalm.dev/156) - 8:6 - Operand of type true is always truthy

ERROR: [DocblockTypeContradiction](https://psalm.dev/155) - 10:9 - Operand of type false is always falsy

https://psalm.dev/r/892d59e94e

Copy link

I found these snippets:

https://psalm.dev/r/892d59e94e
<?php

/**
 * @param array<string, true> $array
 */
function func(array $array): int
{
	if (!empty($array['stuff'])) return 1;
    
    if (empty($array['more stuff'])) return 2;
    return 3;
}
Psalm output (using commit c488d40):

ERROR: RedundantConditionGivenDocblockType - 8:6 - Operand of type true is always truthy

ERROR: DocblockTypeContradiction - 10:9 - Operand of type false is always falsy

@ArtemGoutsoul
Copy link
Contributor Author

Possibly related: #10716

@kkmuffme
Copy link
Contributor

If you enable https://psalm.dev/docs/running_psalm/configuration/#ensurearraystringoffsetsexist in your config, do you still get this error?

@ArtemGoutsoul
Copy link
Contributor Author

@kkmuffme well, the default setting for ensureArrayStringOffsetsExist is false, so when I set it to true, I only get new errors, no existing errors get removed (saved reports and made a diff).

So these RedundantConditionGivenDocblockType and DocblockTypeContradiction errors are not affected by ensureArrayStringOffsetsExist setting.

Would have been cool to be able to change the config attributes in the paslm.dev playground :)

@weirdan
Copy link
Collaborator

weirdan commented Feb 26, 2024

Would have been cool to be able to change the config attributes in the paslm.dev playground :)

You can change some of them by clicking the Settings button.

@ArtemGoutsoul
Copy link
Contributor Author

@weirdan yup! would be cool to have the rest of them (with their config names). This way it would be easier to reproduce some test cases, or even better a text area to paste a config! and a version selector for regression reproduction!

but that's totally off-topic :)

@kkmuffme
Copy link
Contributor

It's a bug bc of the true values - works fine with isset https://psalm.dev/r/6de6380bf7

Possibly the issue happens from https://github.com/vimeo/psalm/blob/5.x/src/Psalm/Internal/Analyzer/Statements/Expression/EmptyAnalyzer.php#L62 and below
I would check if it's reporting as possibly undefined for line 62 or not and see from there.

Give it a shot for a PR and report back if you need any help.

Copy link

I found these snippets:

https://psalm.dev/r/6de6380bf7
<?php

/**
 * @param array<string, true> $array
 */
function func(array $array): int
{
	if (isset($array['stuff'])) return 1;
    
    if (!isset($array['more stuff'])) return 2;
    return 3;
}
Psalm output (using commit b940c7e):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Feb 27, 2024

I would check if it's reporting as possibly undefined for line 62 or not and see from there.

It's not. It's been discussed in more details here: #10578 (comment)

@ArtemGoutsoul
Copy link
Contributor Author

@weirdan oops, didn't notice #10578

It feels like this issue is a duplicate of that one.

@weirdan
Copy link
Collaborator

weirdan commented Feb 28, 2024

Most likely it is, but I've no problem to keep both open until we figure out this for sure

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

Successfully merging a pull request may close this issue.

3 participants