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

Scope: use scope->getConstant instead #3666

Merged
merged 4 commits into from
Dec 20, 2024
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 25, 2024

while doing some local tests, I realized the constant name was wrong.

I am not sure how to test this in isolation, therefore submitting without a test.
I think this will be covered after we use $scope->getPhpVersion() in e.g. return type extensions.

I realized there is a bug, as when testing non-namespaced files the PHP_VERSION_ID was not properly found within the scope

@staabm staabm changed the title Scope: use fully qualified constant reference Scope: use scope->getConstant instead Nov 25, 2024
Comment on lines -5730 to -5732
$versionExpr = new ConstFetch(new Name('PHP_VERSION_ID'));
if (!$this->hasExpressionType($versionExpr)->yes()) {
Copy link
Contributor Author

@staabm staabm Nov 25, 2024

Choose a reason for hiding this comment

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

this previous code does not work in files without a namespace, e.g.

<?php // lint >= 7.4

if (PHP_VERSION_ID < 80000) {
	\PHPStan\dumpType(PHP_VERSION_ID);
}

the dumpType returns the correct value, but the scope uses a different expression name

$scope->debug();
array (
  '\\PHP_VERSION_ID (Yes)' => 'int<50207, 79999>',
  'native \\PHP_VERSION_ID (Yes)' => 'int<50207, 79999>',
)

vs. (note: file contains a namespace)

<?php // lint >= 7.4

namespace ANamespace;

if (PHP_VERSION_ID < 80000) {
	\PHPStan\dumpType(PHP_VERSION_ID);
}
$scope->debug();
array (
  'PHP_VERSION_ID (Yes)' => 'int<50207, 79999>',
  'native PHP_VERSION_ID (Yes)' => 'int<50207, 79999>',
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't \PHP_VERSION_ID const fetch enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, thats the point of this PR

@staabm staabm marked this pull request as ready for review November 25, 2024 13:39
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

We should:

  1. Add (maybe private) method getGlobalConstantType in MutatingScope.
  2. Refactor hasConstant logic that creates the correct ConstFetch
  3. Reuse the logic from 2) in our getPhpVersion method
  4. Also add tests because without tests I don't trust any change :)

@staabm staabm force-pushed the typo branch 3 times, most recently from 747d62e to 8095786 Compare November 25, 2024 18:56
return [
[
'int<80000, 80499>',
__DIR__ . '/data/global-scope-constants.php',
Copy link
Member

Choose a reason for hiding this comment

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

And a test for a namespaced file would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here you are

@ondrejmirtes ondrejmirtes merged commit f6c556f into phpstan:2.0.x Dec 20, 2024
320 of 321 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the typo branch December 20, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants