Fix support for classes named after pseudotypes in phpdoc

[stof]

The non-standard pseudotype int|float will be used only if the resolved class does not exist.

Closes phpstan/phpstan#4039

[stof]

@ondrejmirtes any guidance about where I should add a test covering this ?

[stof]

Note that once this is released, the doc at https://phpstan.org/writing-php-code/phpdocs-basics#classes-named-after-internal-php-types should be updated to stop talking about Number (which is not an internal php type btw)

[ondrejmirtes]

Hi, I'd be pretty careful with this. It might be a BC break for some people. I'd rather be it this was a TypeNodeResolverExtension enabled via bleedingEdge in conditionalTags config section.

Also, we could (and should) do it for more similar types like Resource and Numeric. And it should also work in resolveGenericTypeNode with Number<Foo> etc.

[stof]

@ondrejmirtes AFAICT, Number<Foo> will work automatically as resolveGenericTypeNode calls resolveIdentifierTypeNode.

If this approach about preferring an existing class over the pseudotype for valid class names works for you, applying it to Resource and Numeric indeed makes sense. I would even say it should also apply to other pseudo-types that can be valid class names (types with a - in them cannot be class names): Scalar and Never

[ondrejmirtes]

I'd like some additional logic:

1. Never use it in a file without namespace - fall back to the pseudotype.
2. Always use it when there's a use for that exact name.
3. Ask ReflectionProvider only when we are in a namespace and there's no use for that name.

You can test this in NodeScopeResolver::testFileAsserts by adding a new data provider and see how the existing data providers work like.

[stof]

Hi, I'd be pretty careful with this. It might be a BC break for some people. I'd rather be it this was a TypeNodeResolverExtension enabled via bleedingEdge in conditionalTags config section.

do you have any example for that ?

[ondrejmirtes]

TypeNodeResolverExtension: https://github.com/phpstan/phpstan-phpunit/blob/master/src/PhpDoc/PHPUnit/MockObjectTypeNodeResolverExtension.php

conditionalTags: https://github.com/phpstan/phpstan-strict-rules/blob/70b84f65f2b55427785c470cf8c2cbea5470e3e2/rules.neon#L44-L48

But feel free to implement and test the logic as it is. I have a feeling that the NodeScopeResolverTest will not see the stuff registered behind bleedingEdge tag right now.

[stof]

• Never use it in a file without namespace - fall back to the pseudotype.

• Always use it when there's a use for that exact name.

@ondrejmirtes what is the expected order for these 2 rules ? What should I do if a file without namespace contains a use statement ?

[ondrejmirtes]

Use the use statement 😊

[stof]

@ondrejmirtes I pushed an update implementing the new logic. The tests are not yet done though.
I applied it to all identifier types which are valid class names in PHP.

[ondrejmirtes]

Please don't do that for the names that are affected by this warning: https://3v4l.org/f1UF7

[stof]

ah, I was not aware of that warning on PHP 8. I tried defining a class but not typehinting it.

[ondrejmirtes]

@stof Hi, just asking - when will you have time to finish this PR? Thanks.

[stof]

@ondrejmirtes I'll try to do it this evening

[stof]

Looks like resource, boolean and double will trigger a warning in typehints in PHP 8, but only if they are written in lowercase. If you capitalize them, it also opts out of the warning (just like when fully qualifying them).

and it still treat that as a class typehint in all PHP versions. The only impacted thing is how you write the typehint when using such class name (you cannot use an unqualified lowercase name)

[stof]

@ondrejmirtes given that the warning only applies to lowercase names, should I really disqualify them entirely ?
Also, the warning does not forbid treating them as class name (it tells you they were already the case.

[stof]

@ondrejmirtes I added tests covering this. But the tests for classes existing in the same namespace (without use statements) don't work because it looks like the ReflectionProvider used by the tests don't see classes defined in the same file. How should this be wired in tests ?

[ondrejmirtes]

@stof It should be fine, you need to run composer dump-autoload, alternatively vendor/bin/phing tests-fast-static-reflection which should give you the same result.

Also, please use the modern style tests in NodeScopeResolverTest::testFileAsserts() - see the associated data providers, they use assertType() function.

[stof]

@ondrejmirtes I updated to the modern style, and I added a require_once for the file in the data provider, as done in some other tests, which solved the issue with the ReflectionProvider.

[ondrejmirtes]

I love this, I really really do. Looks like it will solve problems people have without actually breaking anything.

One more thing to fix and I'll click the Merge button :)

[ondrejmirtes]

Instead of this leaking of NameScope internals, I'd add NameScope::hasUseAlias().

[stof]

@ondrejmirtes good idea. This is done.