How to typehint resources that are now objects in PHP 8.1?

[sad-spirit]

Consider the following code

class ResultSet
{
    private $pgsqlResult;
    private $affectedRows;
    
    public function __construct($pgsqlResult)
    {
        $this->pgsqlResult  = $pgsqlResult;
        $this->affectedRows = \pg_affected_rows($pgsqlResult);
    }
}

The argument to constructor here is the value returned from e.g. pg_query: it is a resource of type 'pgsql result' in PHP below 8.1 and an instance of \PgSql\Result since PHP 8.1

If I leave the code as the above, then psalm properly complains

INFO: MixedArgument - 10:49 - Argument 1 of pg_affected_rows cannot be mixed, expecting PgSql\Result
https://psalm.dev/r/fa97a533d4

or

INFO: MixedArgument - 10:49 - Argument 1 of pg_affected_rows cannot be mixed, expecting resource
https://psalm.dev/r/fa97a533d4?php=8.0

Since this is either a resource or an instance of \PgSql\Result then the obvious choice is to add the following typehint:

class ResultSet
{
    private $pgsqlResult;
    private $affectedRows;
    
    /**
     * @param resource|\PgSql\Result $pgsqlResult
     */
    public function __construct($pgsqlResult)
    {
        $this->pgsqlResult  = $pgsqlResult;
        $this->affectedRows = \pg_affected_rows($pgsqlResult);
    }
}

Unfortunately this doesn't quite work:

ERROR: PossiblyInvalidArgument - 13:49 - Argument 1 of pg_affected_rows expects PgSql\Result, possibly different type PgSql\Result|resource provided
https://psalm.dev/r/19b9160c58

or

ERROR: UndefinedDocblockClass - 8:15 - Docblock-defined class, interface or enum named PgSql\Result does not exist
https://psalm.dev/r/19b9160c58?php=8.0

Is there a way to properly typehint this without resorting to two separate classes for different versions of PHP? Maybe psalm should ignore non-existence of classes that replace resources in PHP 8.1 when testing on PHP below 8.1?

Can something like resource&\Pgsql\Result typehint be allowed?

[orklah]

Problem is, we want to be able to match anything other than PgSql for people running exclusively on PHP 8.1.

If you start with this contraint, the problem at hand is nearly impossible to handle without a specific config or something like that...

[sad-spirit]

Could it perhaps be extended to support PHP_VERSION_ID and make Psalm to use that + configured PHP target version to infer actual parameter type?

Cool, that'll look a lot less a hack than resource&\Pgsql\Result that I proposed!

[orklah]

The big caveat is that conditional types only work for returns. It's not possible to document that a function must take a resource in PHP <8.1 but PgSql >=8.1

[sad-spirit]

I thought a bit about the issue:

• Ignoring non-existent classes in PHP below 8.1 is not quite needed, stubs for these can easily be added in the project being tested;
• Typehinting resource|\Resource\Replacing\Class for function return value is more or less correct and easy to understand;
• resource&\Resource\Replacing\Class looks like a hack and allowing it will probably need a lot of code changes;
• Tried looking at the code that deals with parsing conditional types, now my eye is twitching a bit.

The best idea IMO is to silence PossiblyInvalidArgument when passing resource|\Resource\Replacing\Class as an argument to function expecting either of these types:

• We know all the classes that replace resources and the PHP version they were introduced in.
• Looking at psalm's code, this issue can only happen in \Psalm\Internal\Analyzer\Statements\Expression\Call\ArgumentAnalyzer::verifyType() and \Psalm\Internal\Analyzer\Statements\Expression\Call\ArrayFunctionArgumentsAnalyzer::checkClosureTypeArgs()

Idea: right before we are going to raise a PossiblyInvalidArgument issue, we can try to "simplify" a union type having both a resource branch and a class branch by removing either a resource branch (PHP >= threshold version) or a class branch (PHP < threshold version) and re-check with the simplified type. If it fits --- no issue raised. I am not sure whether this may prevent catching some actual issues, though. Also the code of the above methods looks scary.

[orklah]

We know all the classes that replace resources and the PHP version they were introduced in.

Right now, we don't. Psalm process callmap files in bulk and there's no way to continue the process further than the current version to see what it would change into in the previous version.

The issue goes further than just PossiblyInvalidArgument. We don't want PHP <8.1 projects calling is_object and Psalm saying it's possible, and we want to be able to catch is_resource on PHP 8.1 to say it's not possible.

Your needs are literally opposed to those of the majority of users. That's why I talked about a config earlier, but thinking about it, stubs on your end would probably be better. You can document those yourself returning resource|PgSql. You'll sacrifice some Psalm inference and bug awareness, but at least, you will stop being bugged about those errors.

[sad-spirit]

The issue goes further than just PossiblyInvalidArgument. We don't want PHP <8.1 projects calling is_object and Psalm saying it's possible, and we want to be able to catch is_resource on PHP 8.1 to say it's not possible.

Thanks for the explanation, now I see your point. This is definitely the right behaviour when analyzing application code targeting the specific PHP version. Unfortunately this doesn't quite work for a library that usually supports several PHP versions and may need to expose underlying resources to that same application code.

BTW

The big caveat is that conditional types only work for returns. It's not possible to document that a function must take a resource in PHP <8.1 but PgSql >=8.1

I think having just the return type may be sufficient for the library case. The library code can still have resource|class argument typehints as it can define stubs and whatnot, but its return values used in app code will have proper version-specific types without requiring stubs and error suppression.