-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix #4637 by duplicating the type definition for DriverManager::getConnection($args)
params
#4638
Fix #4637 by duplicating the type definition for DriverManager::getConnection($args)
params
#4638
Conversation
…er::getConnection($args)` params As per doctrine#4637, `vimeo/psalm:4.6.4` cannot introduce templated parameters within a re-used imported type: this is understandable, since imported types are copy-paste aided by the type checker, but do not carry any template information with them (and it would be too complex to do so anyway). Quoting the initial issue: Minimal reproducer: https://psalm.dev/r/62a0a5854d ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @param array<string,mixed> $params * @param Configuration|null $config The configuration to use. * @param EventManager|null $eventManager The event manager to use. * * @phpstan-param array<string,mixed> $params * @psalm-param Params $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` Which produces: ``` Psalm output (using commit aa854ae): INFO: MixedInferredReturnType - 31:26 - Could not verify return type 'MyConnection' for mkConnection ``` The issue is that the `Param` alias type is not templated: this is potentially a psalm limitation, but also an understandable one, since there is no syntax to specify template arguments for an imported type (AFAIK). Here is a potential fix: https://psalm.dev/r/8ef9909bb9 ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @psalm-param array{ * charset?: string, * wrapperClass?: class-string<T>, * } $params * @psalm-return ($params is array{wrapperClass: mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` It's a bit ugly, but the idea is that we replace `Params` with the concrete definition again, but this time including `T`, so that the type inference can correctly fill the type variable with the given input: ``` Psalm output (using commit aa854ae): No issues! ``` The idea therefore is to re-introduce some duplication at the advantage of better type inference later on. In order to verify that the change had an effect, we introduced a new `tests/Doctrine/StaticAnalysis` directory, to be checked via `vendor/bin/psalm -c psalm-strict.xml`, which will tell us exactly whether there are obvious type-checker issues around our code. Since `psalm.xml.dist` uses `errorLevel="2"`, it is not sufficient to integrate this directory within the normal workflow. Because this project uses a custom github action for running `vimeo/psalm`, it is not really possible to run this in CI, and therefore that part of the patch has been omitted. It is endorsed that maintainers of `doctrine/dbal` in future: 1. commit `composer.lock` 2. set up dependabot to track dependency upgrades 3. use the standard `vimeo/psalm` via `require-dev`, and run the CI static analysis checks from the same environment that runs normal unit/integration tests
0a0b035
to
af2a31e
Compare
These issues seem to be pre-existing: unsure whether/how they should be adjusted. Should I document these keys?
|
The issue does not seem to be a lack of documentation
For instance here, although it's documented that
We know, and you know we know :P Personally I'm not certain that would be best for us given the high level of notifications we get from dependabot for a repository alone (doctrine/coding-standard, although it might be misconfigured?), also, I have spear-headed the removal of
Maybe duplicate the job, and add a step that overwrites the normal psalm config with the one introduced in this PR? Ugly, but I think it's better if all the tests are run. Anyway, thanks for this PR, it looks really great, I wish they were all this clear! |
It does not look that way, see this other very recent PR: #4640 I don't have an explanation though, sorry. |
Feasible, will try.
Meh, notifications can be silenced, but that's none of my biz :P Meanwhile, I'll re-adjust the failing test so that assertions no longer fail type-checking there. |
… types match up The normal psalm checks run in CI are not sufficient, since they run with level 2 checks (too lax), and therefore we need a dedicated action to be run when verifying the contents of `tests/Doctrine/StaticAnalysis`, which is designed to verify type inference and type-checking details provided by the library.
…hole data structure equality While it is true that such tests are accessing unknown/undocumented array keys that are not part of the psalm type definitions, we still need to verify equality on the retrieved data structure, in order to avoid accidentally introducing a BC break while messing with them. This change therefore: * makes the type-checker a bit happier (was failing before, due to undocumented keys) * makes the tests a bit more strict around potential undocumented keys in these arrays
All green, ready to go! /cc @greg0ire |
Thanks, @Ocramius! |
Fixes #4637
As per #4637,
vimeo/psalm:4.6.4
cannot introduce templated parameters within a re-used importedtype: this is understandable, since imported types are copy-paste aided by the type checker, but do
not carry any template information with them (and it would be too complex to do so anyway).
Quoting the initial issue:
Minimal reproducer: https://psalm.dev/r/62a0a5854d
Which produces:
The issue is that the
Param
alias type is not templated: this is potentially a psalm limitation, but also an understandable one, since there is no syntax to specify template arguments for an imported type (AFAIK).Here is a potential fix:
https://psalm.dev/r/8ef9909bb9
It's a bit ugly, but the idea is that we replace
Params
with the concrete definition again, but this time includingT
, so that the type inference can correctly fill the type variable with the given input:The idea therefore is to re-introduce some duplication at the advantage of better type inference later on.
In order to verify that the change had an effect, we introduced a new
tests/Doctrine/StaticAnalysis
directory,to be checked via
vendor/bin/psalm -c psalm-strict.xml
, which will tell us exactly whether there are obvioustype-checker issues around our code. Since
psalm.xml.dist
useserrorLevel="2"
, it is not sufficient to integratethis directory within the normal workflow.
Because this project uses a custom github action for running
vimeo/psalm
, it is not really possible to run thisin CI, and therefore that part of the patch has been omitted.
It is endorsed that maintainers of
doctrine/dbal
in future:composer.lock
vimeo/psalm
viarequire-dev
, and run the CI static analysis checks from the sameenvironment that runs normal unit/integration tests