vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.55k stars 662 forks source link

Reference typed properties don't need docblocks #8839

Open nmeri17 opened 1 year ago

nmeri17 commented 1 year ago

I didn't want to spam issues but was asked to create a new ticket on this comment . I've also checked the psalm.xml docs and can't find settings for this.

I have a class where a property is updated to a reference type somewhere within the code. I expect Psalm to type the property to that reference type, like so:

class A {
      public ?MyType $u = null; 

Instead, I get,

class A {
    /**
     * @var CapsuleManager|null
     */
 public $u; 

This works fine for scalar types and OCCASSIONALLY for reference types (I can see some properties set to object instead of their real types). My composer.json is set to 8.1, but I still set it on the CLI just to be on the safe side. It still does the same thing. I'm not sure this can be tested with one class since the types are imported from other libraries. You can clone the project and attempt psalm on this file https://github.com/nmeri17/suphle/blob/dbeeaa7e639863b587dbd40d7d308325e2958f02/src/Adapters/Orms/Eloquent/OrmBridge.php. You should get this:

<?php
    namespace Suphle\Adapters\Orms\Eloquent;

    use Suphle\Hydration\Container;

    use Suphle\Contracts\{Database\OrmDialect, Config\Database, Bridge\LaravelContainer, Services\Decorators\BindsAsSingleton};

    use Suphle\Contracts\Auth\{UserHydrator as HydratorContract, AuthStorage, UserContract};

    use Illuminate\Database\{DatabaseManager, Capsule\Manager as CapsuleManager, Connection};

    class OrmBridge implements OrmDialect, BindsAsSingleton {

        private array $credentials;

  /**
   * @var Connection|null
   */
  private $connection;

  /**
   * @var CapsuleManager|null
   */
  private $nativeClient;

        public function __construct (Database $config, private readonly Container $container, private readonly LaravelContainer $laravelContainer) {

            $this->credentials = $config->getCredentials();
        }

        /**
         * @return string
         *
         * @psalm-return OrmDialect::class
         */
        public function entityIdentity ():string {

            return OrmDialect::class;
        }

        /**
         * @param {drivers} Assoc array with shape [name => [username, password, driver, database]]
        */
        public function setConnection (array $drivers = []):void {

            $nativeClient = $this->nativeClient = new CapsuleManager;

            if (empty($drivers)) $connections = $this->credentials;

            else $connections = $drivers;

            foreach ($connections as $name => $credentials)

                $nativeClient->addConnection($credentials, $name);

            $this->connection = $nativeClient->getConnection();
        }

        /**
         * @return Connection
        */
        public function getConnection ():object {

            if (is_null($this->connection)) $this->setConnection();

            return $this->connection;
        }

        /**
         *  Obtain lock before running
        */
        public function runTransaction(callable $queries, array $lockModels = [], bool $hardLock = false) {

            return $this->connection->transaction(function () use ($lockModels, $hardLock, $queries) {

                $this->applyLock($lockModels, $hardLock);

                return $queries();
            });
        }

        public function applyLock(array $models, bool $isHard):void {

            $firstModel = current($models);

            $modelName = $firstModel::class;

            $primaryField = $firstModel->getKeyName();

            $lockingMethod = $isHard ? "lockForUpdate": "sharedLock";

            (new $modelName)->$lockingMethod()->whereIn( // combine user query state into special locking builder to avoid applying update to all rows

                $primaryField, array_map(fn($model) => $model->$primaryField, $models)
            )->get();
        }

        /**
         * {@inheritdoc}
        */
        public function registerObservers (array $observers, AuthStorage $authStorage):void {

            if (empty($observers)) return;

            $this->laravelContainer->instance(AuthStorage::class, $authStorage); // guards in those observers will be relying on this contract

            foreach ($observers as $model => $observer) {

                $this->laravelContainer->bind($observer, function ($app) use ($observer) {

                    return $this->container->getClass($observer); // just to be on the safe side in case observer has bound entities
                });

                $model::observe($observer); // even if we hydrate an instance, they'll still flatten it, anyway
            }
        }

        public function selectFields ($builder, array $filters):object {

            return $builder->select($filters);
        }

        public function addWhereClause( $model, array $constraints) {

            return $model->where($constraints);
        }

        public function getNativeClient ():object {

            return $this->nativeClient;
        }

        public function getUserHydrator ():\Suphle\Hydration\A {

            $hydrator = $this->container->getClass(UserHydrator::class);

            $hydrator->setUserModel(

                $this->container->getClass(UserContract::class)
            );

            return $hydrator;
        }

        /**
         * {@inheritdoc}
        */
        public function restoreConnections (array $modules):void {

            foreach ($modules as $descriptor)

                $descriptor->getContainer()->getClass(OrmDialect::class);
        }
    }
?>

Another related issue is that typed methods don't need additional docblocks. Maybe it's just me. I'll prefer to turn it off if such setting exists. I have this method that should be skipped:

public function entityIdentity ():string {

    return OrmDialect::class;
}

But then, it produces this:


/**
    * @return string
    *
    * @psalm-return OrmDialect::class
    */
public function entityIdentity ():string {

    return OrmDialect::class;
}
psalm-github-bot[bot] commented 1 year ago

Hey @nmeri17, can you reproduce the issue on https://psalm.dev ?

nmeri17 commented 1 year ago

Can anyone help with this, please? Cc @orklah @muglug

orklah commented 1 year ago

For you first issue, check https://psalm.dev/docs/manipulating_code/fixing/#safety-features, I think your answer will be here.

For you second issue, there is no config for that. Your function returns a type that is more specific that what you documented, it's Psalm's job to add the most precise type in docblock