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

fix: check for php v8.1.0 in ReturnTypeAnalyzer (rather than v8.10.0) #9472

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pereorga
Copy link
Contributor

@pereorga pereorga commented Mar 8, 2023

No description provided.

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Mar 8, 2023
@orklah
Copy link
Collaborator

orklah commented Mar 8, 2023

seems like it makes this code fails:

$generator = static function (): \Generator
{
    yield from [];
};

we should probably add a test with this

cc @kkmuffme , do you have an idea why this fails?

@weirdan
Copy link
Collaborator

weirdan commented Mar 9, 2023

Can we declare $analysis_php_version_id as int<5_06_00, 8_03_00> to prevent such typos?

@pereorga
Copy link
Contributor Author

pereorga commented Mar 9, 2023

Can we declare $analysis_php_version_id as int<5_06_00, 8_03_00> to prevent such typos?

Should it be 7_04_00? I don't see 5_06_00 mentions in the codebase. And composer.json says minimum version is 7.4.

(By the way, I'm seeing that in ProjectAnalyzer.php's setPhpVersion() there is a pattern allowing for 5.4 and 5.5 PHP versions. Mentioning it in case these can be removed.)

Ideally, I guess versions should be written in a single place, so they do not need to be updated on every PHP release, on dozens of places. Or maybe a test could be written to enforce these versions.

Going to the point, if I try to declare this in Codebase.php:

    /** @var int<5_06_00, 8_03_00> */
    public $analysis_php_version_id = PHP_VERSION_ID;

I get:

int<5_06_00, 8_03_00> is not a valid type (Invalid type '5_06_00')

If I put:

    /** @var int<50600, 80300> */
    public $analysis_php_version_id = PHP_VERSION_ID;

I get:

ERROR: InvalidPropertyAssignmentValue - src/Psalm/Codebase.php:303:39 - $this->analysis_php_version_id with declared type 'int<50600, 80300>' cannot be assigned type 'int<1, max>' (see https://psalm.dev/145)
    public $analysis_php_version_id = PHP_VERSION_ID;

If I try to suppress InvalidPropertyAssignmentValue here, I get around 80 errors, such as:

ERROR: MissingPropertyType - src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayCombineReturnTypeProvider.php:114:20 - Property Psalm\Codebase::$analysis_php_version_id does not have a declared type (see https://psalm.dev/045)
            return $statements_source->getCodebase()->analysis_php_version_id >= 8_00_00

I'm not familiar with the psalm codebase (nor in using psalm a lot, yet, to be honest), and I'm a bit unsure how to best address this (without specifying a declaration that include PHP versions, in many places)

And the same regarding the impact of this change, as unfortunately I don't fully follow the code.

@weirdan weirdan force-pushed the fix/ReturnTypeAnalyzer-php-version branch from 00a71c7 to ee21f44 Compare March 12, 2023 02:23
@weirdan
Copy link
Collaborator

weirdan commented Mar 12, 2023

BCC failure is ok I think.

/** @var int */
public $analysis_php_version_id = PHP_VERSION_ID;
/** @var int<50400,80299> */
public $analysis_php_version_id;
Copy link
Contributor Author

@pereorga pereorga Mar 13, 2023

Choose a reason for hiding this comment

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

Off-topic, but should it be a space after the comma? Not sure phpcs has rules that support enforcing this kind of things

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either way is fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants