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

Make AbstractSchemaManager covariant to its template argument #6334

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

zerkms
Copy link
Contributor

@zerkms zerkms commented Mar 13, 2024

Q A
Type bug
Fixed issues

Summary

<?php
declare(strict_types=1);

abstract class AbstractPlatform {}

class PostgreSQLPlatform extends AbstractPlatform {}

/**
 * @template T of AbstractPlatform
 */
abstract class AbstractSchemaManager {}

/**
 * @extends AbstractSchemaManager<PostgreSQLPlatform>
 */
class PostgreSQLSchemaManager extends AbstractSchemaManager {}

function createSchemaManager(): AbstractSchemaManager
{
    return new PostgreSQLSchemaManager();
}

$schemaManager = createSchemaManager();

if (!$schemaManager instanceof PostgreSQLSchemaManager) {}
ERROR: [RedundantCondition](https://psalm.dev/122) - 25:6 - AbstractSchemaManager<AbstractPlatform> cannot be identical to PostgreSQLSchemaManager

vs

<?php
declare(strict_types=1);

abstract class AbstractPlatform {}

class PostgreSQLPlatform extends AbstractPlatform {}

/**
 * @template-covariant T of AbstractPlatform
 */
abstract class AbstractSchemaManager {}

/**
 * @extends AbstractSchemaManager<PostgreSQLPlatform>
 */
class PostgreSQLSchemaManager extends AbstractSchemaManager {}

function createSchemaManager(): AbstractSchemaManager
{
    return new PostgreSQLSchemaManager();
}

$schemaManager = createSchemaManager();

if (!$schemaManager instanceof PostgreSQLSchemaManager) {}
No issues!

Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Jun 11, 2024
@zerkms
Copy link
Contributor Author

zerkms commented Jun 11, 2024

🤷

@greg0ire
Copy link
Member

Can you please add something inside the static-analysis directory reproducing the issue described in your original message?

@zerkms
Copy link
Contributor Author

zerkms commented Jun 11, 2024

I'm about to leave to a vacation so will append those whenever I return :-)

@greg0ire
Copy link
Member

Enjoy your vacation!

@zerkms
Copy link
Contributor Author

zerkms commented Jul 30, 2024

Finally I'm here and provided the requested statical analysis test :-)

Without the suggested improvement the test fails with

ERROR: RedundantCondition - static-analysis/a.php:21:6 - Doctrine\DBAL\Schema\AbstractSchemaManager<Doctrine\DBAL\Platforms\AbstractPlatform> cannot be identical to Doctrine\DBAL\Schema\PostgreSQLSchemaManager (see https://psalm.dev/122)
if (!$schemaManager instanceof PostgreSQLSchemaManager) {

greg0ire
greg0ire previously approved these changes Jul 31, 2024
derrabus
derrabus previously approved these changes Jul 31, 2024
@greg0ire
Copy link
Member

Does this apply only to dbal 4?

@zerkms
Copy link
Contributor Author

zerkms commented Jul 31, 2024

@greg0ire it applies to every implementation that defines it as @template T of AbstractPlatform.

I see that all 3.x and 5.x do that too.

@greg0ire
Copy link
Member

greg0ire commented Aug 1, 2024

We use a merge up workflow, please retarget the pr to 3.x

@zerkms zerkms changed the base branch from 4.0.x to 3.8.x August 1, 2024 08:37
@zerkms zerkms dismissed stale reviews from derrabus and greg0ire August 1, 2024 08:37

The base branch was changed.

@zerkms zerkms changed the base branch from 3.8.x to 4.0.x August 1, 2024 08:39
@zerkms zerkms changed the base branch from 4.0.x to 3.8.x August 1, 2024 08:40
@zerkms
Copy link
Contributor Author

zerkms commented Aug 1, 2024

Done

@greg0ire greg0ire merged commit 8a7e9b7 into doctrine:3.8.x Aug 1, 2024
78 of 79 checks passed
@greg0ire greg0ire added this to the 3.8.7 milestone Aug 1, 2024
@greg0ire
Copy link
Member

greg0ire commented Aug 1, 2024

Thanks @zerkms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants