laminas / laminas-inputfilter

Normalize and validate input sets from the web, APIs, the CLI, and more, including files
https://docs.laminas.dev/laminas-inputfilter/
BSD 3-Clause "New" or "Revised" License
41 stars 28 forks source link

Static analysis tools may not understand typehints using Psalm virtual types #75

Closed gszy closed 1 year ago

gszy commented 1 year ago

For example (though this may be the only case of this issue in this project, I haven’t checked in detail yet) in InputFilterProviderInterface.php:17, the return type of InputFilterProviderInterface::getInputFilterSpecification() is specified as InputFilterSpecification|CollectionSpecification using the common @return annotation. These virtual types are in turn defined for Psalm in InputFilterInterface.php:38-47 (using @psalm-import-type and @psalm-type). Any tool that does not use Psalm typehints may (should?) fail to understand the return type of the method. For example, PHPStan thinks the type is never, so from its point of view, it’s impossible to implement the method (in a Form or Fieldset subclass, for example).

ivomasterche commented 1 year ago

Same for Laminas\InputFilter\InputFilter /**

Looking for type InputSpecification, not accepting array (VS Code with PHPTools - showing warning) . (https://discourse.laminas.dev/t/vs-code-doesnt-recognize-psalm-import-type-inputspecification/3090/5)

Ocramius commented 1 year ago

I would say that these problems are to be fixed in the tooling, not here.

Value specifications are already quite popular, such as in monolog/monolog

These features are both used (and useful) in both phpstan/phpstan and vimeo/psalm, and indeed, editors will need to catch up, at some point, but that's us pushing the boundaries of type improvements over here, not a bug :)

gszy commented 1 year ago

In case of Logger.php, there are some PHPStan-specific type annotations:

$ grep 'type\b' Logger.php
 * @phpstan-type Level Logger::DEBUG|Logger::INFO|Logger::NOTICE|Logger::WARNING|Logger::ERROR|Logger::CRITICAL|Logger::ALERT|Logger::EMERGENCY
 * @phpstan-type LevelName 'DEBUG'|'INFO'|'NOTICE'|'WARNING'|'ERROR'|'CRITICAL'|'ALERT'|'EMERGENCY'
 * @phpstan-type Record array{message: string, context: mixed[], level: Level, level_name: LevelName, channel: string, datetime: \DateTimeImmutable, extra: mixed[]}

That are used in some PHPStan-specific param and return annotations, for example:

$ grep '\b\(Level\|LevelName\|Record\)\b' Logger.php | grep 'phpstan-\(param\|return\)' | wc -l
9

But are not used in any non-PHPStan-specific annotations:

$ grep '\b\(Level\|LevelName\|Record\)\b' Logger.php | fgrep -v phpstan
            throw new InvalidArgumentException('Level "'.$level.'" is not defined, use one of: '.implode(', ', array_keys(static::$levels)));
     * @param  string|int                        $level Level number (monolog) or name (PSR-3)
            throw new InvalidArgumentException('Level "'.$level.'" is not defined, use one of: '.implode(', ', array_keys(static::$levels) + static::$levels));
            throw new InvalidArgumentException('Level "'.var_export($level, true).'" is not defined, use one of: '.implode(', ', array_keys(static::$levels) + static::$levels));

So, in this case, PHPStan would use its advanced typehints, while other tools would not need to worry about them.

Ocramius commented 1 year ago

I used to think that way too: having the more advanced types defined in @psalm- and @phpstan- -prefixed annotations only.

Practically, the ecosystem needs a push forward, and if you want a problem to be noticed, you make it bigger 👍

I suggest you raise an issue on the repositories tracking progress for your type inference toolchain of choice instead.