Remove InvalidScalarArgument (BC break)

[muglug]

This could be controversial! I'm happy to close if you think it might be too disruptive for Psalm 5.

InvalidScalarArgument originally came about because, in non-strict mode, PHP is more lenient with those scalar type conversions.

If you take the example given the example code, and add a strict_types declare you get InvalidArgument (example).

But that's not universal — docblock types are treated differently than runtime typehints. This can make some bugs seem less harmful even though they still lead to runtime errors (albeit with a level of indirection).

InvalidScalarArgument has also never played comfortably with PossiblyInvalidArgument — the former often suppresses the latter in an unintuitive fashion.

This PR removes it in favour of InvalidArgument/PossiblyInvalidArgument — whichever is more appropriate.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/83cfce18a5

<?php declare(strict_types=1);

function foo(int $i) : void {}
function bar(string $s) : void {
    if (is_numeric($s)) {
        foo($s);
    }
}

Psalm output (using commit e9ad61e):

ERROR: InvalidArgument - 6:13 - Argument 1 of foo expects int, numeric-string provided

https://psalm.dev/r/001ac14f96

<?php declare(strict_types=1);

function foo(int $i) : void {}
function bar(string $s) : void {
    if (is_numeric($s)) {
        foo($s);
    }
}

/** @param int $i */
function foo_soft($i) : void {
	foo($i);
}
function bar_soft(string $s) : void {
    if (is_numeric($s)) {
        foo_soft($s);
    }
}

Psalm output (using commit e9ad61e):

ERROR: InvalidArgument - 6:13 - Argument 1 of foo expects int, numeric-string provided

ERROR: InvalidScalarArgument - 16:18 - Argument 1 of foo_soft expects int, numeric-string provided

[AndrolGenhald]

What do you think about leaving it as an issue, but being more strict about where it's emitted? Change it so string doesn't scalar-coerce to int but numeric-string still can? I think most of the other scalar coercions are more straightforward, it seems like string to int or float is the one that causes the most actual problems (although "0" to false is a bit weird too).

Is it actually still useful to have the issue if it's more strict like that, or would it be less useful to the point that it's not worth maintaining?

[orklah]

In practice, whatever can help users with huge baseline to solve the bigger issues first is a win in my book. This seems to go in the other direction for me.

For users that have small baselines, this has no impact or very little.

So, we would need to have a big reason for wanting to drop that internally for it to be worth it. You say it plays weirdly with PossiblyInvalidArgument so it could be one. Maybe we need to keep that in mind to not stick to the current behaviour if this is an issue for future refactors

[muglug]

Ok, in that case I'd argue for something slightly different: keep InvalidScalarArgument, but ensure it only fires for non-docblock types (where PHP is sure to be lenient). I'll draw up a different PR to see how that'd look.

[muglug]

Closing this in favour of #7188 — maybe it could be considered again for Psalm 6, but I have no great attachment to it.