barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.2k stars 1.16k forks source link

Generate `numeric-string` type for model fields with a `decimal` cast #1582

Closed ekisu closed 2 days ago

ekisu commented 2 months ago

Summary

Right now, model fields with a decimal type cast are annotated with the string type. This is correct, however typing them with a numeric-string type would help static analysis tools to better understand the code, and also to not flag valid code as incorrect. An example would be the following code:

/**
 * @property-read string $value
 */
readonly class ModelWithString {
    public function __construct(
        public string $value,
    ) {}
}

/**
 * @property-read numeric-string $value
 */
readonly class ModelWithNumericString {
    public function __construct(
        public string $value,
    ) {}
}

$a = new ModelWithString('1.23');
$result = $a->value + 3;

$b = new ModelWithNumericString('1.23');
$result2 = $b->value + 3;

In the above snippet, PHPStan flags the addition operation using the ModelWithString value property as invalid, but is able to check that the operation using the ModelWithNumericString value property is valid code (link to PHPStan's playground).

I am willing to work on this.

mfn commented 2 months ago

Can you check if you can change it via hooks (see readme)?

ekisu commented 2 months ago

I managed to change the properties' types to numeric-string using a model hook, however I couldn't quite figure out how to keep the same nullability as the original ones :thinking:. Here's the code:

class ChangeDecimalCastsToNumericString implements ModelHookInterface
{
    public function run(ModelsCommand $command, Model $model): void
    {
        $casts = $model->getCasts();

        foreach ($casts as $fieldName => $cast) {
            if (!str_starts_with($cast, 'decimal')) {
                continue;
            }

            $command->setProperty($fieldName, 'numeric-string');
        }
    }
}

Even if it's possible to add this behavior as a model hook, I do believe it would be useful to have it by default, as it's widely supported by static analysis tools and IDEs/LSPs.

barryvdh commented 2 months ago

What is the support for this type?

ekisu commented 2 months ago

On static analysis tools, it's supported by:

On IDE/LSPs, it's supported by:


On a second thought, I think that numeric will be more useful than numeric-string in this case, because the decimal cast allows us to write float|int into the property, and this isn't really allowed when using numeric-string, as shown in the following snippet (link to PHPStan's playground). Support should be pretty much the same/wider than numeric-string.

/**
 * @property string $value
 */
class ModelWithString {
    public function __construct(
        public string $value,
    ) {}
}

/**
 * @property numeric-string $value
 */
class ModelWithNumericString {
    public function __construct(
        public string $value,
    ) {}
}

/**
 * @property numeric $value
 */
class ModelWithNumeric {
    public function __construct(
        public string $value,
    ) {}
}

$a = new ModelWithString('1.23');
$a->value = $a->value + 3;
$a->value = '4.56';
$a->value = 4.56;

$b = new ModelWithNumericString('1.23');
$b->value = $b->value + 3;
$b->value = '4.56';
$b->value = 4.56;

$c = new ModelWithNumeric('1.23');
$c->value = $c->value + 3;
$c->value = '4.56';
$c->value = 4.56;