Bug: missing InvalidArgument error when passing false to true param

[kkmuffme]

https://psalm.dev/r/2c91682853

When passing false to the param, it should report as InvalidArgument just like it does for when we pass 123

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/2c91682853

<?php

/**
 * @param true|string $arg
 * @return void
 */
function foo( $arg = true ) {
    if ( $arg === true ) {
        echo "Yes";
    }
}

foo( false );
foo( 123 );

Psalm output (using commit 93267f2):

ERROR: InvalidArgument - 14:6 - Argument 1 of foo expects string|true, but 123 provided

[orklah]

Interesting, this is caused by this line:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php

Line 864 in 1c19260

true,

the param "ignore_false" is set to true by default on this check. It's a little surprising because we have other mechanisms to ignore false for internal functions so I don't get why it's put to true here.

Maybe try to push a PR putting that to false and let's see where the CI goes?

[kkmuffme]

See the PR created, it fails in tons of places then.
Looks like it's related to https://psalm.dev/docs/running_psalm/configuration/#ignoreinternalfunctionfalsereturn - maybe if making that "true" in line 864 conditional on that setting would be correct?

shepherd runs with https://psalm.dev/docs/running_psalm/configuration/#ignoreinternalfunctionfalsereturn true or false?

[orklah]

Yeah, that's the other mechanism I was talking about earlier.

Could you try to change this line:

psalm/src/Psalm/Internal/Type/Comparator/UnionTypeComparator.php

Line 79 in 1c19260

if ($input_type_part instanceof TFalse && $ignore_false) {

into
if ($input_type_part instanceof TFalse && ($ignore_false || $input_type->ignore_falsable_issues)) {

When shepherd runs, it is supposed to ignore falsable from internal functions

[kkmuffme]

Done, looks better but it seems there are some underlying logic issues with InvalidArgument, InvalidScalarArgument and PossiblyFalseArgument, since they have some overlap.
However there are some random tests failing, which shouldn't fail, e.g. https://github.com/vimeo/psalm/actions/runs/4152953868/jobs/7184270305

I also tried removing the first commit with the suggestion of #9267 (comment), however then those that fail for some weird reason also fail, so there seems to be some issue there.

Feel free to check/debug/fix this further, as I currently don't have time to contribute.

[orklah]

Yep, that was not the correct fix.

Can you try

$input_type->ignore_nullable_issues,
$input_type->ignore_falsable_issues,

instead of

true,
true

on the call here:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php

Line 864 in 1c19260

true,

(and reverting the rest)

Apparently, it's done like that on other calls. It seems to fix your case and another test that failed on the previous fixes, let's see if the rest of CI pass

[kkmuffme]

Changed now, but there are tons of tests that fail (however for some it looks like they should fail and the tests are buggy)

[orklah]

Yeah, looks much better IMHO.

I think there's some cases where echo doesn't accept null that would need to get changed to reduce obstacles, but the rest seems legit at first glance

[orklah]

Oh, there's something wrong though: https://github.com/vimeo/psalm/actions/runs/4155594465/jobs/7188799901#step:5:41
This is reported as PossiblyInvalidArgument but it should be a PossiblyNullArgument (as it seems it was before)

Somehow, it seems it's the same issue that makes null be reported with echo. This was handled here before:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php

Line 1077 in 1c19260

if (!$param_type->isNullable() && $cased_method_id !== 'echo' && $cased_method_id !== 'print') {

but it doesn't seem to get there anymore. Could you take a look?

[kkmuffme]

Sorry, I cannot check that any further until some time next month

[orklah]

No worries, seems like this bug is very old, it can wait!