vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

pslam suggesting not-helpful reference page #9131

Open cabbey opened 1 year ago

cabbey commented 1 year ago

if you have a bit of code such as this:

function foo(int $length): string
{
    return random_bytes($length);
}

https://psalm.dev/r/be5350cc96

Even though the signature for random_bytes() is just int, psalm somehow knows that it's expecting a positive, non-zero, value there, which leads to an error message since the caller has only asserted that it's an int: ERROR: ArgumentTypeCoercion - src/File.php - Argument 1 of random_bytes expects positive-int, but parent type int provided. Psalm then refers to https://psalm.dev/193 (which expands to https://psalm.dev/docs/running_psalm/issues/ArgumentTypeCoercion/ today.)

That page however has no useful information about the actual issue in this specific scenario where users are using language primitives. To solve this, I would suggest either:

  1. a different page be linked that talks about the pslam internal types like positive-int and declarations like @psalm-param or @psalm-var
  2. that page be expanded on to talk about the above.

I would think the first of those would be preferable, but I don't know at the point in the code where the error is generated, if psalm has enough information to know if it's a user class or a primitive type.

For the benefit of anyone who lands on this bug in the future searching on the error message, our solution was a mix of:

/**
 * @psalm-param positive-int $length
 */
function foo(int $length): string
{
    return random_bytes($length);
}

and

function foo(int $length): string
{
    /** @psalm-var positive-int $length */
    return random_bytes($length);
}

depending on the context and how much of a headache it meant to propagate that internal psalm pseudo-primitive out into our codebase.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/be5350cc96 ```php , but parent type int provided ```
weirdan commented 1 year ago

The doc page is generated from https://github.com/vimeo/psalm/blob/master/docs/running_psalm/issues/ArgumentTypeCoercion.md

Feel free to submit a PR expanding it.

cabbey commented 1 year ago

Ok, I take it then that forking off the case for primitives to a different issue is not easily doable? I'll see if I can take a pass at a proposed improvement.

weirdan commented 1 year ago

We used to distinguish between scalar and non-scalar arguments, but it was removed not too long ago. Frankly, I wouldn't want to reintroduce something like that, as it tends to lead to a combinatorial explosion of issue types. E.g. when we treat docblock/non-docblock types and scalar/non-scalar types differently, we could end up with ArgumentTypeCoercion, ScalarArgumentTypeCoercion, DocblockScalarArgumentTypeCoercion and DocblockArgumentTypeCoercion issues.