Recognize constants as constant types #1163

rvanvelzen · 2022-04-01T15:36:52Z

Closes phpstan/phpstan#6160. This was just a wild idea. Probably not the best way forward, but at least it shows the potential

rvanvelzen · 2022-04-01T15:39:53Z

tests/PHPStan/Analyser/data/conditional-types-constant.php

+abstract class Test
+{
+	/**
+	 * @return (PHP_MAJOR_VERSION is 8 ? true : false)


Obviously this breaks on PHP < 8 💥

you should run this test only on php8, or/and add an php7 alternative test-case with a different expectation.

staabm · 2022-04-01T15:52:58Z

src/PhpDoc/TypeNodeResolver.php

 		return new ObjectType($stringName);
 	}

+	private function mightBeConstant(string $name): bool
+	{
+		return preg_match('(^[A-Z_][A-Z0-9_]*$)', $name) > 0;


since A and B or similar short-names might be considered a constant in this case, I think tryResolveConstant should at first try whether there is a class defined with this name. Only if not, try whether its actually a constant.

what do you think?

Agreed, constant can be tried only when it's UPPER_CASE and class of that name does not exist.

staabm · 2022-04-01T16:00:00Z

I love it. this kills 2 birds with one stone.

it implements phpstan/phpstan#6160 and fixes one of the psalm-special-cases within the conditional-type language phpstan/phpstan#3853 (comment)

ondrejmirtes

I hate to bring the bad news but:

I like to resolve constants here to solve Support global constants in union types phpstan#6160
But I don't think it makes sense to support this in conditional types. The code here reports int<5, max>: https://phpstan.org/r/853028b5-3d52-4f51-ae64-1655458ef879 So typehinting PHP_MAJOR_VERSION in a PHPDoc also has to resolve to int<5, max> thanks to dynamicConstantNames (https://phpstan.org/config-reference#constants).

Which makes it useless for conditional types. But it's fine for me - having PHP_MAJOR_VERSION conditional in a stub is useful only for built-in PHP functions, and these can be solved by writing code in an extension. I don't want to have a small cosmetic advantage at a cost of doing some horrible hacks to make this work in stubs :)

ondrejmirtes · 2022-04-02T13:36:07Z

Also wildcard support would be a great addition, some people expect that to work (phpstan/phpstan#6972). Needs modification in phpdoc-parser too.

See phpstan#1163 for discussion. `TypeNodeResolver` needs to be able to resolve constants the same way as `MutatingScope` does

See #1163 for discussion. `TypeNodeResolver` needs to be able to resolve constants the same way as `MutatingScope` does

rvanvelzen · 2022-04-07T15:53:48Z

Also wildcard support would be a great addition, some people expect that to work (phpstan/phpstan#6972). Needs modification in phpdoc-parser too.

If you don't mind I'd like to tackle that separately. This PR has become quite large enough as-is 🥲

src/Analyser/ContainerConstantResolver.php

ondrejmirtes · 2022-04-07T16:06:22Z

I agree, we can leave the wildcards for a later time :)

ondrejmirtes

Perfect! Can you please check that NameScope doesn't contain use function uses as they are useless for it?

ondrejmirtes · 2022-04-08T06:57:48Z

Checking the code they probably aren't there thanks to TYPE_NORMAL.

ondrejmirtes · 2022-04-08T06:58:58Z

src/PhpDoc/TypeNodeResolver.php

@@ -361,9 +366,36 @@ private function resolveIdentifierTypeNode(IdentifierTypeNode $typeNode, NameSco
 			return new ErrorType();
 		}

+		if (!$this->mightBeConstant($typeNode->name) || $this->getReflectionProvider()->hasClass($stringName)) {


I think this should just be if ($this->getReflectionProvider()->hasClass($stringName)) {.

Because if something looks like a constant FOO_BAR but there's a class FOO_BAR, I want ObjectType of the class, not the constant.

This should still always result in the class taking precedence, but does a slightly cheaper heuristic check.

It's covered in the test as well: https://github.com/phpstan/phpstan-src/pull/1163/files#diff-f60365c259ff216f4d228e0c46729817e679144be2a7bf2e7395ace7fd344284R47

I don't think it hits that condition:

private function mightBeConstant(string $name): bool { return preg_match('((?:^|\\\\)[A-Z_][A-Z0-9_]*$)', $name) > 0; }

This just matches UPPER_CASE names, right? But the constant in question has CamelCaps namespace.

Because when I read that code in case of FOO_BAR the first return new ObjectType($stringName); is skipped because mightBeConstant() === true. And then tryResolveConstant is successful because the constant exists. So constant takes precedence over class.

The namespace isn't considered, only the last part of $name (so BAR in that case). If the constant took precedence the type in the test would be 10, instead of the class name (ConstantPhpdocType\BAR).

Now it makes sense to me :) I'm ready to merge this, thanks.

rvanvelzen · 2022-04-08T07:02:15Z

Can you please check that NameScope doesn't contain use function uses as they are useless for it?

It shouldn't because only TYPE_NORMAL and TYPE_CONSTANT are considered, and use function is TYPE_FUNCTION.

Closes phpstan/phpstan#6160. This was just a wild idea. Probably not the best way forward, but at least it shows the potential

ondrejmirtes · 2022-04-08T10:57:09Z

Thank you! Please send an update to the documentation: https://phpstan.org/writing-php-code/phpdoc-types (add a new section "Global constants").

Documentation for phpstan/phpstan-src#1163

vudaltsov · 2024-04-21T12:20:21Z

Did you consider introducing a new constant<MY_CONSTANT> type constructor? That would simplify things a lot and allow to use not only global constants, but any other.

ondrejmirtes · 2024-04-21T12:22:05Z

@vudaltsov You're commenting on a two year old PR. You should open a new feature request and describe your use case.

rvanvelzen commented Apr 1, 2022

View reviewed changes

staabm reviewed Apr 1, 2022

View reviewed changes

staabm mentioned this pull request Apr 1, 2022

Add support for conditional return type phpstan/phpstan#3853

Closed

rvanvelzen marked this pull request as ready for review April 2, 2022 06:59

ondrejmirtes requested changes Apr 2, 2022

View reviewed changes

rvanvelzen marked this pull request as draft April 4, 2022 08:13

rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Apr 7, 2022

Extract resolving dynamic constants into a helper

412bfbc

See phpstan#1163 for discussion. `TypeNodeResolver` needs to be able to resolve constants the same way as `MutatingScope` does

rvanvelzen mentioned this pull request Apr 7, 2022

Extract resolving dynamic constants into a helper #1184

Merged

ondrejmirtes pushed a commit that referenced this pull request Apr 7, 2022

Extract resolving dynamic constants into a helper

35964eb

See #1163 for discussion. `TypeNodeResolver` needs to be able to resolve constants the same way as `MutatingScope` does

rvanvelzen force-pushed the constant-reference branch from 5b11ca8 to db4d492 Compare April 7, 2022 15:52

ondrejmirtes requested changes Apr 7, 2022

View reviewed changes

src/Analyser/ContainerConstantResolver.php Outdated Show resolved Hide resolved

ondrejmirtes approved these changes Apr 8, 2022

View reviewed changes

ondrejmirtes requested changes Apr 8, 2022

View reviewed changes

rvanvelzen added 9 commits April 8, 2022 10:25

Recognize constants as constant types

a2790ae

Closes phpstan/phpstan#6160. This was just a wild idea. Probably not the best way forward, but at least it shows the potential

Separate test cases for PHP 7 and PHP 8

42567e7

Check class existence and add tests

f53cd7e

Prevent unnecessary class existence checks

d908669

Simplify

2bf6499

Fix dynamic constants

cbf7842

Support const aliases properly

a3af606

Simplify

36163f1

Make condition clearer

97f7026

rvanvelzen force-pushed the constant-reference branch from d07a223 to 97f7026 Compare April 8, 2022 08:25

rvanvelzen marked this pull request as ready for review April 8, 2022 08:42

ondrejmirtes approved these changes Apr 8, 2022

View reviewed changes

ondrejmirtes merged commit f0bfd4d into phpstan:1.6.x Apr 8, 2022

rvanvelzen deleted the constant-reference branch April 8, 2022 11:27

rvanvelzen mentioned this pull request Apr 8, 2022

Add documentation for global constants phpstan/phpstan#7006

Merged

ondrejmirtes pushed a commit to phpstan/phpstan that referenced this pull request Apr 26, 2022

Add documentation for global constants

8a65968

Documentation for phpstan/phpstan-src#1163

phpstan locked and limited conversation to collaborators Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recognize constants as constant types #1163

Recognize constants as constant types #1163

rvanvelzen commented Apr 1, 2022

rvanvelzen Apr 1, 2022

staabm Apr 1, 2022 •

edited

staabm Apr 1, 2022 •

edited

ondrejmirtes Apr 1, 2022

staabm commented Apr 1, 2022

ondrejmirtes left a comment

ondrejmirtes commented Apr 2, 2022

rvanvelzen commented Apr 7, 2022

ondrejmirtes commented Apr 7, 2022

ondrejmirtes left a comment

ondrejmirtes commented Apr 8, 2022

ondrejmirtes Apr 8, 2022

rvanvelzen Apr 8, 2022

ondrejmirtes Apr 8, 2022

rvanvelzen Apr 8, 2022

ondrejmirtes Apr 8, 2022

rvanvelzen commented Apr 8, 2022

ondrejmirtes commented Apr 8, 2022

vudaltsov commented Apr 21, 2024

ondrejmirtes commented Apr 21, 2024

Recognize constants as constant types #1163

Recognize constants as constant types #1163

Conversation

rvanvelzen commented Apr 1, 2022

rvanvelzen Apr 1, 2022

Choose a reason for hiding this comment

staabm Apr 1, 2022 • edited

Choose a reason for hiding this comment

staabm Apr 1, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes Apr 1, 2022

Choose a reason for hiding this comment

staabm commented Apr 1, 2022

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes commented Apr 2, 2022

rvanvelzen commented Apr 7, 2022

ondrejmirtes commented Apr 7, 2022

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes commented Apr 8, 2022

ondrejmirtes Apr 8, 2022

Choose a reason for hiding this comment

rvanvelzen Apr 8, 2022

Choose a reason for hiding this comment

ondrejmirtes Apr 8, 2022

Choose a reason for hiding this comment

rvanvelzen Apr 8, 2022

Choose a reason for hiding this comment

ondrejmirtes Apr 8, 2022

Choose a reason for hiding this comment

rvanvelzen commented Apr 8, 2022

ondrejmirtes commented Apr 8, 2022

vudaltsov commented Apr 21, 2024

ondrejmirtes commented Apr 21, 2024

staabm Apr 1, 2022 •

edited

staabm Apr 1, 2022 •

edited