Native types shouldn't try to use the current namespace

Hi,

It seems phpdoc-parser tries to use the files' namespace even for native types. Not sure how to best explain this, let me do my best.

There this really odd issue on the Safe library where phpstan + larastan would throw an error on any library using the Safe package. At first I thought the error was on larastan but after they fixed what I thought was the root cause, the error was still there, so after some more debugging (thank $DEITY for xdebug) I found out that the problem was on the library (or on phpdoc-parser, it could be fixed on both places).

Long story short, the library is basically a bunch of files redeclaring native php functions, the issue is that whenever there's a phpdoc on one of their files using @param array or @return array, since all function files are namespaced under Safe, the parser tries to use the type Safe\array, which unfortunately has a PSR4 match on their array.php file. The bug appeared when the library was used together with larastan because larastan checks if any of the parameters is a subclass of one of Laravel's Eloquent classes on one of their extensions, triggering the second loading of the same file (doing (new ObjectType(Model::class))->isSuperTypeOf(new ObjectType($nameScope->resolveStringName('Safe\array')))->yes()). I (erroneously) thought that adding \ to the array will force it to be on the global namespace but it turns out that \array is not a valid type for phpstan.

Anyway, I believe the native types from PHP should take precedence when being parsed (same than the actual PHP behaviour), maybe with some sort of list to be compared with when parsing the types from @param and @return, although I'm not fully familiar with the implementation to make a proper suggestion.

Please, let me know if there are any doubts so I can explain further, not my best day today 😛

This is a parser. It does not do semantic resolution, it just returns AST.

PHPStan then corrently resolves types in TypeNodeResolver.

which unfortunately has a PSR4 match on their array.php

That's not unfortunate. That's simply wrong. PSR-4 does not allow to mix classes and functions in the same directory, i.e. to safe library needs to fix their autoloading defintion as suggested in thecodingmachine/safe#253 (comment)

I guess this issue is just misfiled and should be in phpstan/phpstan.

But try phpstan/phpstan:1.6.x-dev and enable bleedingEdge https://phpstan.org/blog/what-is-bleeding-edge, there should be no side effects of autoloading.

Additionaly Larastan is also wrong to assume that IdentifierTypeNode can always be resolved as a class name. That is incorrect. It should use TypeNodeResolver.

@ondrejmirtes

Ok, thank you very much for your answers and sorry for filing the bug on the wrong place. I assumed it was the parser who read infered Safe\array from the parsed type. I guess it can be closed then.

@ondrejmirtes Do you want me to reopen this on the phpstan repo? I'll test the latest version as soon as I get some time off work.

Thanks to both for the quick replies!

Also, just for the record, Safe\array does not exist as a class, so there is no mix between classes and functions on the library. But phpstan (or larastan, not sure anymore) assumes the type and tries to load it as a class (which does not exists), but the autoloader then loads the file with the functions again and causes the "cannot redeclare" error.

I assumed it was the parser who read infered Safe\array from the parsed type.

The "wrong resolution" part of the problem is literally one line above the link you posted https://github.com/nunomaduro/larastan/blob/v2.1.4/src/Types/GenericEloquentBuilderTypeNodeResolverExtension.php#L36.

If Larastan fix this issue then there would be no attempt to load Safe\array class.

there is no mix between classes and functions on the library

They use same directory which PSR-4 does not support. Repeated call to class_exist(\Safe\array::class) must not result in error. Well working autoloader should never result in error when checking for existence of non-existing class. It should just return false.

And now I wonder what is actually the best way Larastan can fix the resolution issue. Maybe PHPStan should expose a constant with all known keywords, so Larastan could replace

if ($innerTypeNode instanceof IdentifierTypeNode
    && $this->provider->hasClass($nameScope->resolveStringName($innerTypeNode->name))
    && (new ObjectType(Model::class))->isSuperTypeOf(new ObjectType($nameScope->resolveStringName($innerTypeNode->name)))->yes()
) {
    ...
}

with sth like

if ($innerTypeNode instanceof IdentifierTypeNode
    && !isset(TypeNodeResolver::KEYWORDS[strtolower($innerTypeNode->name)]
    && $this->provider->hasClass($nameScope->resolveStringName($innerTypeNode->name))
    && (new ObjectType(Model::class))->isSuperTypeOf(new ObjectType($nameScope->resolveStringName($innerTypeNode->name)))->yes()
) {
    ...
}

@JanTvrdik

@JanTvrdik Thanks for the explanation, I'm still finding my way around phpstan and AST, so I wasn't 100% sure what the code was doing.

The one thing I still don't understand is (and sorry if I'm missing something obvious), there are no classes (afaics) on the generated/ directory so what do you mean by that? Do you mean the classes and the functions they all live on the same namespace? Or is it because of the exceptions that live under generated/Exceptions? What should be the proper way? Maybe I'm looking in the wrong place, but couldn't find any mention of function-only files on the PSR4 spec.

Sorry to waste your time, I'm learning a lot in the journey trying to debug and fix this issue.

@j3j5

@j3j5 Do you understand that calling class_exists should never result in error? It's explicitly mentioned in point 4 of PSR-4 specification

Autoloader implementations MUST NOT throw exceptions, MUST NOT raise errors of any level, and SHOULD NOT return a value.

The Safe's autoloading definition is incorrect, because it configures Composer autoloader in a way that makes it possible for some calls of class_exists (such as repeated call of class_exist(\Safe\array::class)) to result in error.

The Safe library needs to

either stop mixing classes and functions in the same directory used for PSR-4 autoloading
or stop using PSR-4 autoloader and use the more flexible classmap autoloader instead.

Yes, I understand what you mean now, I misinterpreted point number 4 earlier as being the responsibility of the autoloader implementation but indeed, it's as much responsibility of the class/file being loaded using PSR-4. I'm going to try to implement this suggestion on a PR for them.

Again, thanks for you time. I appreciate your responses.

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.

Navigation Menu

Repository navigation

Native types shouldn't try to use the current namespace #117

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Native types shouldn't try to use the current namespace #117

Description

Activity

JanTvrdik commented on Apr 19, 2022

ondrejmirtes commented on Apr 19, 2022

JanTvrdik commented on Apr 19, 2022

j3j5 commented on Apr 19, 2022

j3j5 commented on Apr 19, 2022

JanTvrdik commented on Apr 19, 2022

JanTvrdik commented on Apr 19, 2022

j3j5 commented on Apr 19, 2022

JanTvrdik commented on Apr 20, 2022

j3j5 commented on Apr 20, 2022

github-actions commented on May 22, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions