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

ParadoxicalCondition when assignment happens inside conditionals #4374

Closed
alecwcp opened this issue Oct 20, 2020 · 5 comments
Closed

ParadoxicalCondition when assignment happens inside conditionals #4374

alecwcp opened this issue Oct 20, 2020 · 5 comments
Labels

Comments

@alecwcp
Copy link

alecwcp commented Oct 20, 2020

Psalm detects ParadoxicalCondition (and UnusedVariable) but there is no paradox and the variable is used.

https://psalm.dev/r/e23bd1a389

Looks like this is because of the assignment within the conditions, so psalm sees it as 2 comparisons of the same variable with false, but actually the value of the variable changes between the conditionals due to the assignment.

Normally I would be against assignment in conditionals for readability reasons, but In this case it is actually useful for checking multiple formats and stopping and the first that worked.

We can get around this by changing to

<?php

function checkDateIsRFC3339(?string $dateString): ?\DateTimeImmutable
{
    if (null === $dateString) {
        return null;
    }

    $date1 = \DateTimeImmutable::createFromFormat(DATE_RFC3339, $dateString);
    $date2 = \DateTimeImmutable::createFromFormat(DATE_RFC3339_EXTENDED, $dateString);

    if (false === $date1 && false === $date2) {
        return null;
    }
    if (false !== $date1) {
    	return $date1;
    }

    return $date2;
}

but if possible it seems that supporting the assignments within the conditionals would be potentially beneficial elsewhere.

@psalm-github-bot
Copy link

I found these snippets:

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

function checkDateIsRFC3339(?string $dateString): ?\DateTimeImmutable
{
	if (
    	null === $dateString ||
        	((false === $date = \DateTimeImmutable::createFromFormat(DATE_RFC3339, $dateString)) &&
            	(false === $date = \DateTimeImmutable::createFromFormat(DATE_RFC3339_EXTENDED, $dateString)))
	) {
    	return null;
	}
    return $date;
}
Psalm output (using commit 6678071):

ERROR: ParadoxicalCondition - 7:11 - Found a redundant condition when evaluating assertion (($date is not false) || ($date is not false))

INFO: UnusedVariable - 8:25 - Variable $date is never referenced

@muglug
Copy link
Collaborator

muglug commented Oct 20, 2020

Here's a better working rewrite (IMO): https://psalm.dev/r/3d21ee76cc

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/3d21ee76cc
<?php

function checkDateIsRFC3339(?string $dateString): ?\DateTimeImmutable
{
	if (!$dateString) {
        return null;
    }
    
    $date = \DateTimeImmutable::createFromFormat(DATE_RFC3339, $dateString)
        ?: \DateTimeImmutable::createFromFormat(DATE_RFC3339_EXTENDED, $dateString);
    
    return $date ?: null;
}
Psalm output (using commit 6678071):

No issues!

@muglug muglug added the bug label Oct 20, 2020
@muglug
Copy link
Collaborator

muglug commented Oct 20, 2020

Here's a lighter reproducer for the UnusedVariable issue

<?php

function foo(string $str): ?int {
    if (!($pos = strpos($str, "a")) && !($pos = strpos($str, "b"))) {
        return null;
    }
    
    return $pos;
}

and for the ParaxodicalCondition:

<?php

function foo(string $str): ?int {
    if (!$str || (!($pos = strpos($str, "a")) && !($pos = strpos($str, "b")))) {
        return null;
    }
    
    return $pos;
}

@muglug muglug closed this as completed in f72e2d7 Oct 20, 2020
@alecwcp
Copy link
Author

alecwcp commented Oct 20, 2020

Thanks for the improved rewrite @muglug 👍
And also a fix in under 3 hours! Psalm's such a brilliant tool, coupled with such great support too!

danog pushed a commit to danog/psalm that referenced this issue Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants