phan icon indicating copy to clipboard operation
phan copied to clipboard

`$users->count() > 0` does not imply `$users->first()` is non-null: $PhanTypeExpectedObjectPropAccess reported on Laravel collection items

Open jsncrdnl opened this issue 3 years ago • 4 comments

Hi

I'm getting a weird error when fetching first element of a collection (constructed from an eloquent db model):

        $users = User::all();
        if ($user->count() > 0) {
            // @var User $user
            $user = $users->first();
            return $user->id;
        }

The error I get:

PhanTypeExpectedObjectPropAccess Expected an object instance when accessing an instance property, but saw an expression $user with type TValue|null

The @var annotation doesn't make a change...
The only thing that works is asserting (locally) the variable like this:

        $users = User::all();
        if ($user->count() > 0) {
            // @var User $user
            $user = $users->first();
            if (null === $user) {
                throw new \Exception('user is null, it should not');
            } else if (!$user instanceof User) {
                throw new \Exception('$user should be an instance of User');
            }
            return $user->id;
        }

I also tried to externalize the logic in another class to make the code shorter but it not preventing the phan error reporting ...

        $users = User::all();
        if ($user->count() > 0) {
            // @var User $user
            $user = $users->first();
            // throws an exception if variable is null or if not an instance of the provided class
            TypeAssertion::assert($user, User::class);

            return $user->id;
        }

jsncrdnl avatar Sep 20 '22 07:09 jsncrdnl

Yes, Phan doesn't attempt to infer relationships between conditions on calls of different method names - there's no relationship between count() and first(), the same way there wouldn't be a relationship between $users->foo() and $users->bar()

The @var annotation doesn't make a change...

https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#doc-blocks

Doc blocks must be enclosed in comments of the form /** ... */ starting with /** and ending with */. Annotations won't be read from comments with any other format. This will cause you frustration.

TypeAssertion::assert must be declared with @phan-assert annotations - phan doesn't try to infer these automatically from the implementation. https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#assertions

e.g. based on tests/plugin_test/src/072_custom_assertions.php from phan's source code (I updated the wiki just now)

<?php
/**
 * Assert that $mixed is an instance of the class/interface $className
 *
 * @template TClassName
 * @param class-string<TClassName> $className
 * @param mixed $mixed the value we're making the assertion on
 * @phan-assert TClassName $mixed
 * @throws InvalidArgumentException
 */
function my_assert_instance(string $className, $mixed): void {
    if (!$mixed instanceof $className) {
        throw new InvalidArgumentException("expected to find instance of $className but failed");
    }
}

$var = json_decode($argv[2]);
my_assert_instance(stdClass::class, $var);
// Phan now asserts $var is an stdClass

TysonAndre avatar Sep 21 '22 00:09 TysonAndre

        $users = User::all();
        if ($user->count() > 0) {

nit: In the pseudocode you wrote based on whatever internal code you had, you're calling $users to get all users, but then calling $user->count() instead of $users->count(), though regardless $users->count() > 0 would have no impact on analysis either

TysonAndre avatar Sep 21 '22 00:09 TysonAndre

            // @var User $user

Also, in tools/IDEs that did expect inline type comments (Phan doesn't), many of them would only accept /** @var User $user */, not a line comment

TysonAndre avatar Sep 21 '22 00:09 TysonAndre

You're right for the @var annotation, it took shortcut to showcase my issue ..

I used your custom assertion sample for my assertion class and it's not triggering phan errors anymore !
Many thanks about that !

jsncrdnl avatar Sep 21 '22 07:09 jsncrdnl