Skip to content
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

Psalm 5.11.0 #5830

Closed
wants to merge 1 commit into from
Closed

Psalm 5.11.0 #5830

wants to merge 1 commit into from

Conversation

derrabus
Copy link
Member

The Psalm upgrade is a massacre. 🙈

This is my current WIP state, in case anyone feels like completing it.

@derrabus
Copy link
Member Author

How am I supposed to read and understand errors like this one?

ERROR: ArgumentTypeCoercion - src/DriverManager.php:209:38 - Argument 1 of Doctrine\DBAL\DriverManager::createDriver expects array{charset?: string, dbname?: string, defaultTableOptions?: array<string, mixed>, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, replica?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, serverVersion?: string, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, unix_socket?: string, url?: string, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>}, but parent type array{charset?: string, dbname?: string, defaultTableOptions?: array<string, mixed>, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, defaultTableOptions?: array<string, mixed>, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, replica?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, serverVersion?: string, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, unix_socket?: string, url?: string, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>}, replica?: array<array-key, array{charset?: string, dbname?: string, defaultTableOptions?: array<string, mixed>, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, keepReplica?: bool, keepSlave?: bool, master?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, memory?: bool, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, primary?: array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}, replica?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, serverVersion?: string, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, unix_socket?: string, url?: string, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>}>, serverVersion?: string, sharding?: array<string, mixed>, slaves?: array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>, unix_socket?: string, url?: string, user?: string, wrapperClass?: class-string<Doctrine\DBAL\Connection>} provided (see https://psalm.dev/193)

cc @orklah – sorry for the ping, but I'm really stuck here. I'm getting error messages that don't fit into my 🧠. 🙈

@greg0ire
Copy link
Member

@derrabus that's a big error! Here is a more legible version: https://www.diffchecker.com/It5OguUg

@derrabus
Copy link
Member Author

Thanks @greg0ire. Take a look at the failures, there's more where this came from. 🙈

@orklah
Copy link

orklah commented Dec 12, 2022

For a somewhat better clarity:

EDITED @greg0ire gave a better version :)

With Psalm 5, array{} is now sealed so if the first form is expected, you should have no extra keys in the second form. There's a few extra keys here, especially inside the primary like defaultTableOptions, keepReplica, keepSlave, master, primary (inside primary??)
then, you have some extra keys on root level, replica, serverVersion, sharding, slaves, unix_socket, url, user, wrapperClass.

So you have different options:

  • either all those keys are irrelevant for your called method, then adding , ... just before the closing brake of your root shape and your root > primary shape should be enough
  • or some of those keys should not even be there (I'm thinking about root > primary > primary maybe?) and they should be removed
  • or they're useful to the called method and they should be added in the @param

@orklah
Copy link

orklah commented Dec 12, 2022

Note: While I know it's yet additional work, and OSS time is not free, adding shapes declared as aliases would help a lot here

For example, just in the error you posted, there are 13 occurences of the type
array<array-key, array{charset?: string, dbname?: string, default_dbname?: string, driver?: 'ibm_db2'|'mysqli'|'oci8'|'pdo_mysql'|'pdo_oci'|'pdo_pgsql'|'pdo_sqlite'|'pdo_sqlsrv'|'sqlite3'|'sqlsrv', driverClass?: class-string<Doctrine\DBAL\Driver>, driverOptions?: array<array-key, mixed>, host?: string, password?: string, path?: string, pdo?: PDO, platform?: Doctrine\DBAL\Platforms\AbstractPlatform, port?: int, unix_socket?: string, user?: string}>

Having 13 times 451 chars replaced by 13 "ConnexionInfo" would have reduced this error by more than half

@greg0ire
Copy link
Member

Thanks a lot @orklah !
Regarding the params, I think we already have those declared:

* @psalm-type OverrideParams = array{
* charset?: string,
* dbname?: string,
* default_dbname?: string,
* driver?: key-of<self::DRIVER_MAP>,
* driverClass?: class-string<Driver>,
* driverOptions?: array<mixed>,
* host?: string,
* password?: string,
* path?: string,
* pdo?: \PDO,
* platform?: Platforms\AbstractPlatform,
* port?: int,
* user?: string,
* unix_socket?: string,
* }
* @psalm-type Params = array{
* charset?: string,
* dbname?: string,
* defaultTableOptions?: array<string, mixed>,
* default_dbname?: string,
* driver?: key-of<self::DRIVER_MAP>,
* driverClass?: class-string<Driver>,
* driverOptions?: array<mixed>,
* host?: string,
* keepSlave?: bool,
* keepReplica?: bool,
* master?: OverrideParams,
* memory?: bool,
* password?: string,
* path?: string,
* pdo?: \PDO,
* platform?: Platforms\AbstractPlatform,
* port?: int,
* primary?: OverrideParams,
* replica?: array<OverrideParams>,
* serverVersion?: string,
* sharding?: array<string,mixed>,
* slaves?: array<OverrideParams>,
* url?: string,
* user?: string,
* wrapperClass?: class-string<Connection>,
* unix_socket?: string,
* }

But maybe there are occurrences that are not replaced yet, we will have to check.

@derrabus
Copy link
Member Author

Thank you for chiming in @orklah. Once I take a look at the formatted error, I think I somewhat know what to do. My problem is rather that Psalm throws a single-line 10 KB error message at me for which I need external tooling.

adding shapes declared as aliases would help a lot here

But doesn't Psalm inline those aliases when rendering the error message? This big shape that we see in the error message is already composed of type aliases and I don't see them in the output at all.

@orklah
Copy link

orklah commented Dec 12, 2022

Yeah, my bad, Psalm does inline everything, this is unfortunate as it would help a lot. I'll create a ticket, even if I'm not sure what can be done now that I think about it.

@derrabus derrabus changed the title Psalm 5.2.0 Psalm 5.3.0 Dec 18, 2022
@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from 9e0e8f1 to aa8b860 Compare December 19, 2022 09:42
@derrabus derrabus changed the title Psalm 5.3.0 Psalm 5.4.0 Dec 29, 2022
</MethodSignatureMustProvideReturnType>
<NoValue>
<errorLevel type="suppress">
<!-- We're checking for invalid input. -->
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like a bug. You shouldn't end up with NoValue while checking types from docblock. Do you know at which line it is emitted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give you a reproducer: https://psalm.dev/r/614cec5f0f

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think it happens because the type is both from signature and from docblock, I'll check that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error occurs because I first eliminate all possible values (according to the docblock) for a variable and afterwards use its value when rendering the exception message.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I know, the thing is that when a type comes from a docblock, when every type is refuted, Psalm assign the mixed type to it. It only applie the never type when the type came from signature. Here you have a signature but you actually have a more detailed type in docblock

@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from 149b11a to 26dd1c9 Compare January 3, 2023 18:42
@derrabus derrabus changed the base branch from 3.5.x to 3.6.x January 3, 2023 18:42
@derrabus
Copy link
Member Author

derrabus commented Jan 3, 2023

I figured whatever I so here will be too heavy for the bugfix branch. Rebased to 3.6.x, so I can start cherry-picking some fixes.

@derrabus
Copy link
Member Author

derrabus commented Jan 3, 2023

Apparently, Psalm has become super-picky about the mixed type:

The type list<mixed> is more general than the declared return type list<string> for Doctrine\DBAL\Schema\SQLServerSchemaManager::listSchemaNames

This happens because Psalm can't possibly know that the given SQL will result in a string result and that's okay. However, it should be okay to use a mixed value as something else unless I enable really strict settings. For instance, PHPStan would not complain about silent mixed-to-string casts unless it runs on the strictest level 9.

Also, it's a bit annoying that this kind of error is apparently always reported twice, once for the return type declaration (MoreSpecificReturnType) and one for the actual return statement (LessSpecificReturnStatement). Now I get two pointless errors to silence. 😢

@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from b7966bf to 2744b69 Compare January 3, 2023 19:28
@orklah
Copy link

orklah commented Jan 3, 2023

However, it should be okay to use a mixed value as something else unless I enable really strict settings

Agree. It's actually a bug: vimeo/psalm#9049

it's a bit annoying that this kind of error is apparently always reported twice

That confused me too, but if you account the fact that Psalm can fix some code, it makes sense you'd not want to treat that kind of error the same way. Fixing LessSpecificReturnStatement would probably involve some kind of casting or check for the return whereas fixing MoreSpecificReturnType would involve broadening the return type in phpdoc or signature.

You could suppress one of those globally on the project depending on where you want to get the error if you want

@derrabus
Copy link
Member Author

derrabus commented Jan 3, 2023

However, it should be okay to use a mixed value as something else unless I enable really strict settings

Agree. It's actually a bug: vimeo/psalm#9049

Oh good. I was about to get mad about that one. Thanks. 😅

it's a bit annoying that this kind of error is apparently always reported twice

That confused me too, but if you account the fact that Psalm can fix some code, it makes sense you'd not want to treat that kind of error the same way.

Okay, if you put it this way, it makes sense. 👍🏻

@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from 21083b0 to dc94fc9 Compare February 26, 2023 17:07
@derrabus derrabus changed the title Psalm 5.6.0 Psalm 5.7.7 Feb 26, 2023
@derrabus derrabus changed the base branch from 3.6.x to 3.7.x March 6, 2023 23:02
@derrabus derrabus changed the title Psalm 5.7.7 Psalm 5.8.0 Mar 13, 2023
@derrabus derrabus changed the title Psalm 5.8.0 Psalm 5.9.0 Mar 31, 2023
@derrabus
Copy link
Member Author

derrabus commented Mar 31, 2023

Nice, Psalm finally reports problematic array keys instead of in addition to throwing gigantic type declarations in my face. Looks like someone could pick up the work again.

Probably me, right? 😅

@derrabus
Copy link
Member Author

derrabus commented Apr 2, 2023

Okay, vimeo/psalm#9049 still hurts big time. I can't work with mixed values on array shapes at all.

@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from e9b0c6c to 4263e6f Compare April 3, 2023 17:33
@derrabus
Copy link
Member Author

derrabus commented Apr 3, 2023

Thanks for the fix, @orklah. This allowed me to remove some error suppressions. However, the issue with mixed values in arrays seems only partially fixed. For instance, in our TestUtil class, I still have the issue that an array of mixed is produced while the return type is more specific:

ERROR: MoreSpecificReturnType - tests/TestUtil.php:194:22 - The declared return type 'array{### a very large shape ###}' for Doctrine\DBAL\Tests\TestUtil::mapConnectionParameters is more specific than the inferred return type 'array<### a union of some array keys from that shape ###, mixed>' (see https://psalm.dev/070)
     * @psalm-return Params

Again, the error is valid, just not on the level of strictness that we're running on.

@derrabus derrabus changed the title Psalm 5.9.0 Psalm 5.10.0-dev Apr 3, 2023
@orklah
Copy link

orklah commented Apr 3, 2023

Could you try to replicate on a smaller scale on psalm.dev?

@derrabus
Copy link
Member Author

derrabus commented Apr 3, 2023

I'll try.

I've reported another issue that pops up several times: vimeo/psalm#9604.

@derrabus derrabus force-pushed the bump/psalm-5 branch 2 times, most recently from b605959 to 4ecd445 Compare April 3, 2023 18:44
@derrabus
Copy link
Member Author

derrabus commented Apr 3, 2023

This is the issue I'm seeing: https://psalm.dev/r/6c49e6544a

Again, the error itself is valid, it's just that I get it not matter what I set the errorLevel to which is way to strict for a mixed-to-non-mixed cast.

@derrabus derrabus changed the title Psalm 5.10.0-dev Psalm 5.11.0 May 8, 2023
@derrabus derrabus mentioned this pull request Jun 12, 2023
@derrabus
Copy link
Member Author

Closing in favor of #6061.

@derrabus derrabus closed this Jun 12, 2023
@derrabus derrabus deleted the bump/psalm-5 branch June 12, 2023 08:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants