-
Notifications
You must be signed in to change notification settings - Fork 664
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
Notices when analyzing some PDO-related code #4836
Comments
Hey @morozov, can you reproduce the issue on https://psalm.dev ? |
That definitely makes sense – I assume you're using those stubs? |
Yes (psalm.xml.dist): <stubs>
<file name="vendor/jetbrains/phpstorm-stubs/PDO/PDO.php" />
<file name="vendor/jetbrains/phpstorm-stubs/ibm_db2/ibm_db2.php" />
<file name="vendor/jetbrains/phpstorm-stubs/mysqli/mysqli.php" />
<file name="vendor/jetbrains/phpstorm-stubs/oci8/oci8.php" />
<file name="vendor/jetbrains/phpstorm-stubs/pgsql/pgsql.php" />
<file name="vendor/jetbrains/phpstorm-stubs/sqlsrv/sqlsrv.php" />
</stubs> Primarily on CI in order to avoid having to install all DB extensions for static analysis. |
Currently the populator processes dupllicate methods if it's in the stub phase, but that assumes that the duplicates are in a different file, not the same one. That'll have to change. Not sure about that PHPStorm-only attribute. Hmm. |
Whether it's a proprietary PhpStorm attribute or not, I believe it's still relevant that a given signature may look differently depending on the PHP and/or extension version. To version the stubs, one approach might be to use a separate file for each version and then feed only the relevant ones to the static analyzer or the IDE. It wouldn't require any changes in Psalm but be harder to manage by the maintainers of the stubs and by their users. Another approach would be to have all the stubs in one file annotated with a version constraint (e.g. there's a more classic At this point, it looks reasonable that Psalm introduces the support of the stub version awareness – be it based on the PhpStorm attribute, or |
Yeah, it's not a massive adjustment to support that attribute. Pretty straightforward, and beats picking one arbitrarily. |
@muglug if it's not a huge effort, could it be prioritized? This issue prevents Doctrine DBAL from upgrading to the latest version of stubs which in turn requires adding more items on the issue suppression list every time when a mistake is found in the old stubbed API. |
Thanks for the fix, @weirdan. It seems to be working well with PHP 8.0 (the build on the PR in the description passes) but with PHP 7.4 I still see some similar errors:
|
Does the stub in question have |
Some of the stubs seem to have the annotations. E.g.: /**
* @since 8.0
*/
public function getIterator(){} Not sure what exactly is failing now. If you believe the problem is on the stubs side, I'll try to debug and provide more details. |
v2020.2 you're updating to does not have multiple versions of |
I think it's actually caused by Psalm being confused by version-conditional trait implementation. It uses params and parser params (whatever that is), and expects them to come from the same implementation. But when it analyzes implementation from the false branch it uses param list from the true branch. |
It looks like that. The notices above aren't reproducible on 2777b62 with PHP 7.4 or PHP 8.0. @weirdan I believe this issue can be closed given its subject. As for being able to use the JetBrains stubs which have PHP versions annotated with their proprietary annotations, I want to get rid of the dependency on them entirely since both Psalm and PHPStan have their own stubs. Thank you and @muglug for taking care of the issue. |
The issue can be observed on doctrine/dbal#4465 and is reproducible on any Psalm version from 4.1.1 to 4.3.1. Running
psalm --debug-by-line
yields the following:Where the code in question looks like this:
This looks related to JetBrains/phpstorm-stubs#950 which introduces two methods with the same name and different signatures for different PHP versions.
The text was updated successfully, but these errors were encountered: