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

More precise types in class constants #3530

Closed
jbafford opened this issue Jun 23, 2020 · 4 comments
Closed

More precise types in class constants #3530

jbafford opened this issue Jun 23, 2020 · 4 comments

Comments

@jbafford
Copy link
Contributor

Bug report

An array of class names is not properly detected as class-string.

Code snippet that reproduces the problem

https://phpstan.org/r/e84c46b3-64d2-4326-aa7a-7881f44b2c25

phpstan reports that a method that expects class-string is given string.

Expected output

phpstan should return no error via either of the two following mechanisms:

  • the constant is explicitly defined as having values of class-string<someinterface> via an annotation, which phpstan ignores (class-string not enforced in constants #2798)
  • the constant contains values of class-string, which phpstan can normally infer, but because it has 10 elements, which exceeds CONSTANT_SCALAR_UNION_THRESHOLD, phpstan does not make the inference.

While I would prefer this fixed via fixing #2798 (which would fix this general case entirely), if increasing CONSTANT_SCALAR_UNION_THRESHOLD is the solution you prefer, I should be able to make a PR that adds a configuration option to set it.

@ondrejmirtes
Copy link
Member

So I looked into this and the root issue isn't about CONSTANT_SCALAR_UNION_THRESHOLD, but rather about how we're resolving class constant values: https://github.com/phpstan/phpstan-src/blob/cdf5b8518b9d04277a1489b8859d4a40ef068d26/src/Reflection/ClassConstantReflection.php#L60-L63

The problem is that we have a string value without any context whether it was created as 'A' or A::class. So new ConstantStringType is created with second argument $isClassString=false.

Just to verify - when I hardcoded the second argument to true in ConstantTypeHelper::getTypeFromValue then it worked.

To do this right, we'd have to inspect the AST of the class constant value and not just read the value from the reflection. Whici is possible with the static reflection engine - but it isn't currently deployed for classes that are loaded via autoloader, only for classes that aren't known to registered autoloaders (and are discovered via analysed paths and scanFiles/scanDirectories). So it'd work only in some situations.

Another solution could be to ask for arbitrary strings, whether they are an existing class or not, and set isClassString based on that, but some users' registered autoloaders do not like being asked about nonexistent classes, so I want to avoid this (there were many issues submitted about that).

I see two parts of this task:

  • Fix the problem described above (hard)
  • Start supporting annotations above class constants (easy). I'm finally persuaded about this. PHPStan might be able to figure out that the string retrieved from the array is class-string, but it won't figure out on its own that you only want to have class-string<SyncableEntity>. Of course the class constant type PHPDoc support should also come with a rule that verifies the constant value matches the declared type.

@ondrejmirtes ondrejmirtes changed the title Array of length exceeding CONSTANT_SCALAR_UNION_THRESHOLD does not have type properly detected More precise types in class constants Jun 24, 2020
@phpstan-bot
Copy link
Contributor

@jbafford After the latest commit in dev-master, PHPStan now reports different result with your code snippet:

@@ @@
-46: Parameter #1 $class of method HelloWorld::getRepository() expects class-string, string given.
+No errors

@ondrejmirtes
Copy link
Member

@var above class constants is now read: phpstan/phpstan-src@b932769

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants