Phpstan specific phpdoc annotations in the core code base

Good morning folks,

I’d like to know your opinion on the following matter. A few years back a lot of people started using and embracing phpstan and its features and applied those in the core. We used phpstan to harden the existing code base, find bugs and prevent new bugs. But we also did something without any consent (at least unknown to me). We used phpstan features in regular phpdoc annotations.

Example 1

phpstan is not only capable of reading @var string $foo and understanding the type of a variable whose type is otherwise unknown, phpstan is capable of being fed information beyond the type system of php itself. @var non-empty-string $foo let’s phpstan know that $foo is expected to be non empty which can then lead to phpstan errors if phpstan detects usages of $foo with potentially empty strings.

Those features of phpstan are great but they have a catch. If we start changing regular phpdoc annotations, we do potentially take the possibility to let other tools properly deal with those. Other tools can be phpdoc itself or IDE’s.

And while it’s great to have phpstan and humans understand non-empty-string, it’s equally important to have IDE’s understand that. But if they don’t we actively remove information from the code base.

Example 2

There is another example of code we do have in the TYPO3 code base. GeneralUtility::makeInstance() expects a $className and to have phpstan understand that we do expect a class name here, we changed @param string $className to @param string|class-string<T> $className. At that point, other tools could still just ignore the class-string<T> and know that $className is the php type string. Later, another patch removed the string type and from that day on my IDE doesn’t know what type $className is expected to have and guesses. It ignores <T> , doesn’t know the global class class-string and thinks it must be \TYPO3\CMS\Core\Utility\class-string.

Sure, using PHPStorm, you can use plugins and meta files and what not and have your IDE understand those phpstan features again or you can hope that Jetbrains integrates native support to have the IDE understand those phpstand features out of the box but what if you use a tool that does not have that support? People loose information which once existed.

IMHO

I am always eager to embrace new tools and features but I think we lost track here and went a bit too far and I’d like us to be a bit more conservative, quite literally, here.

Even though very few use it, there still is phpdocumentor with a defined set of supported types (phpDocumentor) and back then before PHP introduced type declarations for return, param and property types, we usually went with the set of defined types of phpdocumentor.

I think we should stick to those with regular @param, @return and @var annotations and add additional information, that is only consumable by phpstan, via @phpstan--variants. I’d like to put emphasis on add here. Instead of

/**
 * @template T of object
 * @param class-string<T> $className
 * @param array<int, mixed> $constructorArguments
 * @return T
 */
public static function makeInstance($className, ...$constructorArguments)
{
}

GeneralUtility::makeInstance() would look like this:

/**
 * @param string $className
 * @param mixed[] $constructorArguments
 * @return object
 *
 * @template T of object
 * @phpstan-param class-string<T> $className
 * @phpstan-param array<int, mixed> $constructorArguments
 * @phpstan-return T
 */
public static function makeInstance($className, ...$constructorArguments)
{
}

Yes, phpstan and humans find duplicate information here but other tools still work as expected.

What’s your take on this?

I’d like to know what you think about it, without going into detail about what specific annotation should remain, look like and/or replaced/enhanced with phpstan features. First, I’d like to get a feeling about how the community thinks about this in general. If the majority doesn’t share my concerns here, there is no need to discuss details.

I am using PHPStorm myself and I’d like to hear what you think about this, what tool(s) you are working with and how those already made changes affect your work and what you think the best solution would be.

1 Like

I’m using Neovim with https://phpactor.readthedocs.io/ (as Language Server) for programming. I don’t face any issues right now with current TYPO3 approach. But I also don’t use phpDocumentor.

Would you like to share your post within TYPO3 Slack Channel #typo3-documentation? I guess they should also provide feedback. Not sure whether there are plans to extract info for docs.typo3.org from the PHPDoc Blocks. I know this is done for Fluid ViewHelper Reference already. And we still have https://api.typo3.org/.

I think that we need to clearly define what the target groups/tools/standards of our annotations. And I also think that we should take the effort of maintaining the annotations into account.

This is what I would suggest (and what’s also my understanding what we’ve done so far):

  • Target mainly PHPStan and the human reader.
  • Do not target specifically any IDEs: If they support a particular annotation, fine, and if not (yet), also fine.
  • Use only @var/@param/@return as much as possible, and only add additional @phpstan-* annotations where this is necessary to not break Extbase & friends).
1 Like

I think that we need to clearly define what the target groups/tools/standards of our annotations.

Still knowing the times of PHP4, to me phpdocs always served the purpose to document variable types when it wasn’t possible through language features itself. Quite naturally, IDE’s hopped on and wrote their code insight engines based on the feature set of phpdocumentor. The majority of my career, this information targeted me as a human reader and my IDE but now I feel we clearly target one specific tool. Which I love to do, UNLESS we remove information for other target groups.

Just a couple more words on the specific @phpstan-annotations. My goal would NOT be to duplicate all information but only introduce @phpstan-variants for phpstan and the human reader when we would otherwise loose type information because there is no type declaration yet.

Do not target specifically any IDEs: If they support a particular annotation, fine, and if not (yet), also fine.

To clarify my POV here as well. I would also not write annotations for specific IDEs, I just don’t want to remove information IDEs were able to use for decades and replace it with something the vendor actively needs to implement. @var string|class-string would still be my preferred choice if we don’t want to use @phpstan--specific annotations.

@var string|class-string would actually remove the information from PHPStan. This is why:

string is a superset of class-string (i.e., all class-string are also a string). So PHPStan would internally reduce this to string and ignore the subset case class-string.

That’s quite unfortunate. :frowning:

So only those potential options remain:

  • Introduce type declaration
  • Introduce additional @phpstan-var annotation, which specifies the subset.

Reference for the class-string|string thing: Conditional return type for container · Issue #7141 · phpstan/phpstan · GitHub

Besides phpstan there is also psalm. Like phpstan it features extensions and prefixed annotations. Both tools are great and share much of the syntax and are alive. For me phpDocumentor is a dead duck. I would not care at all about it.

So I’d use any annotation that goes beyond PHP’s own typing capabilities and is supported by both phpstan and psalm.

If phpstan excels psalm for a given typing a phpstan annotation should be added as well.

Sidenote: the obvious goal is to use PHP types whenever possible. @param string etc should not be necessary at all.

I agree and I think it proves my point to prefix annotations that only a specific tool can understand, even if the syntax of two tools is the same. It can change over time and one tool might not be able to understand a specific kind of syntax any more. I think there’s a reason why phpstan/psalm have and evaluate prefixed annotations, because they introduce syntax that is only understandable by them and humans.

Even if there’s a function with type declarations, I’d suggest going for prefixed annotations.

/**
 * @phpstan-param 0|positive-int $int
 * @psalm-param non-negative-int $int
 *
 * @phpstan-return non-empty-string
 */
public function(int $int): string
{
    // Convert positive int to not empty string.
}

This is an example where both tools have features for the same thing with different syntax. psalm knows non-negative-int which includes 0, phpstan does not, it needs the union type 0|positive-int.

Those are just small differences and very few cases but why not simply be explicit and prefix all annotations that are written for a specific tool, even if we duplicate information. IMHO that’s better than loosing information for other existing tools like we do if we do the following:

 /**
- * @param int $int
+ * @param 0|positive-int $int
  *
- * @return string
+ * @return non-empty-string
  */
 public function($int)
 {
     // Convert positive int to not empty string.
 }

In this example we don’t have type declarations yet because they might be considered breaking in an upcoming TYPO3 release. But even if we do have a fully typed method, I’d add the prefix:

 /**
- * @param int $int
+ * @phpstan-param 0|positive-int $int
  *
- * @return string
+ * @phpstan-return non-empty-string
  */
 public function(int $int): string
 {
     // Convert positive int to not empty string.
 }

Just as a precaution to not feed unparsable information to tools that expect a very specific syntax from @param, @return and so on.

1 Like

Both support now this syntax:

  • int<0, 100>
  • int<min, 100>
  • int<50, max>

So they are on par again in this case.

But as both psalm and PhpStan accept 0|positive-int and int<0,max> there is no need for two annotations. IMHO using two annotations are hard to maintain.

If the Core team “only” uses PhpStan the annotations for psalm would bit-rot very soon.

I don’t think that adding or changing typing in annotations is a breaking change. Yes, it will affect developers who do lint their own code, but it does not affect users who run the code.

Is compatibility with phpDocumentor a goal? The Core does not really make use of any “advanced” features of phpDocumentor. There is no offcial TYPO3 documentation available based on phpDocumentor. So I think it can be ignored when it comes to writing annotations.

So in the end I support @oliklee’s stance: Use unprefixed whenever possible. Use phpstan annotation if it will break Extbase (which should learn to understand native PHP types).

In addition I’d like to see that any annotion used without prefix is also understood by the latest version of psalm (and the version of phpStan the Core version uses).
If Extbase forces a prefixed annotation a psalm annotation may be added as courtesy.

If restricting the unprefixed annotation to be compatible to psalm is to much of a burden then I’m fine with an additional psalm annotation.

But frankly IMHO the Core would much more benefit if native types would be used trhoughout the code and a common base set of phpstan/psalm would be used consistently then fiddling with the latest-and-greatest features of phpstan to express one or few places of the Core exactly. The need for those arises probably because the Core is at that spot what is called “legacy” :wink:

I think a lot of what I want to convey is misunderstood. Maybe it’s the language barrier or lack of communication skills on my side. I will try to clear things up.

That’s great. It would be even better if the PHP world had a new PSR-X, that defined all those possible sub types of all the literals we have, so all tools can consume the same syntax. But we are not there yet and that is my point here. Very often different tools share the same syntax but there is no standard that guarantees that. We see Jetbrains struggle to implement support for generics for years now and I believe it is because the PHP world lacks a standard. So the world leading PHP IDE cannot properly support @template T and other great syntactic sugar. It even chokes on this new syntax when used without prefixed annotations. And until there is a standard everyone agrees on, there is chaos to some degree.

Indeed. Let me clarify: There is no intention from anyone (I know) to introduce psalm as a second static code analyser or to give it the same support phpstan gets. It was just an example to show that phpstan’s syntax is not tool agnostic. psalm can read and parse a lot of it, but not all, and vice versa.

I think it’s a bad thing solely looking at phpstan (as if it sets the new standards) and introduce something like @var class-string and hope that other will simply follow. Best case: In 3 years, all tools can read this. Worst case: In 3 years, only phpstan can read this.

Counter question here: Why do we assume, that all tools suddenly need to be able to read and parse @var class-string? And why are we willing to take away information from long established tools so easily? If a tool was able to read and parse @var string and suddenly it sees @var class-string and it is loosing all type information, this is a very bad situation IMHO.

No, this is another misconception. phpdocumentor is dead and there is no interest in being compatible with it. All I tried to explain was how the current de facto standard for php annotations came to life. Back in the old days, when we didn’t have type declarations anywhere in php, we needed annotations for ourselves. So we, as humans, could keep track of the types variables had at certain points because php didn’t guarantee type safety at all. Back then, the standards we know today established and the most popular tool to read and shape those annotations was phpdocumentor.

So all tools that evolved, implemented the standard. Take symfony/property-info for example. It had to rely on people following the standard to reliably extract variable types from annotations. @var srting wasn’t identified as string, it was just garbage. There is a reason we had tools and IDE’s checking our annotations for errors because other tools relied on it.

What I want to say is that all this history shouldn’t lightheartedly be ignored. @var class-string adds valuable information for humans and two tools while all the rest of long existing tools choke on it. It may not be phpdocumentor but just worlds leading php IDE which tells me makeInstance() does now expect $className to be an instance of \TYPO3\CMS\Core\Utility\class-string. It makes my life as a developer worse, it takes information from my IDE and other tools and I don’t want this to happen when there is a simple alternative with prefixed annotations.

I don’t know what Extbase has to do with it, it understands type declarations. But I also agree on this:

That’s where I want this discussion to head to. We need to have an understanding of what this means. My take on this is: It is possible as long as we don’t take existing information from other tools to replace it with information only phpstan understands.

This further boils down to:
Whenever there is a type declaration, @var, @param, and @return CAN hold information that is only readable by phpstan.

Without a type declaration, @var, @param, and @return MUST NOT hold information that is only readable by phpstan. That information MUST to be put into prefixed annotations.

Please forget Extbase and psalm here. Both are irrelevant here.

I couldn’t agree more but introducing type declarations is considered breaking. Not everywhere, not all the time but it’s too risky to do it in one go. Hence this discussion. We have lots of code without type declarations and where only PHP8 union types actually allow us to proceed further and those are the cases where phpstan syntax in regular annotations takes valuable information from other tools.

1 Like

Well, I’m using psalm and other do as well.That’s the reason why I don’t want to see phpstan-only types in @var etc.

That’s exactly my argument. psalm, like other tools, can gain but also loose information when using phpstan-specific syntax in regular annotations. I personally wouldn’t have any problem with duplicating information i.e. using @psalm- and @phpstan- both even if their syntax is the same.

But what’s your take on my other proposals? What’s a solution everyone benefits from?

1 Like

That’s a solution I can well live with.

Thanks for the extensive summary and showing the problem.

I think we’ve been following this convention, but maybe we need to write it down in our CGL guidelines:

  • Use phpdoc-typehints (such as @param string) when we do not have a typed property/return type OR when there is an additional info that needs to be documented that adds value to the information (example: @return null|int returns 0 if no records are found, or -1 if there are only hidden records found)

In addition, we should do it like this:

  • Add phpstan information as our code base is tested regularly against that tool in addition to the the phpdoc information we adde (see above) - as proposed by Alex (@phpstan- prefix).
  • We do not add psalm-style information to TYPO3 Core, as we do not have a person who wants to maintain it (for more than 8 weeks).

I’ve now been going through phpstan’s baseline for TYPO3 a lot of times in the past 12 months and my summary is:

  • It’s good and it helps to prevent further bugs
  • it does not catch everything
  • Sometimes it’s just wrong :slight_smile: (it’s just a tool)
  • Maintaining this is quite a lot of work and it doesn’t always spark joy for me.
  • I am glad we (you all, not me) added it and maintain it (@oliklee :slight_smile: ) - thanks
2 Likes

I’m late to this party, however I’m opposing to the original proposal and would NOT suggest to use specific tags - thus, NOT using @phpstan-.

Especially when starting to change existing declarations from e.g. @return class-string to @phpstan class-string it probably would break existing functionality/information. For instance there is a TYPO3 Security Scanner (Security Team internal) based on Psalm which might break and behave differently then.

As far as possible, and in case the meaning of tags is not different in PHPstan vs. PsalmPHP, I’d suggest to use the native annotation, without applying prefixes - thus, use @template ...@ instead of @phpstan-template …@, use @param class-string $value instead of @phpstan-param class-string $value.

It’s true that major IDEs are not supporting these kind of tags per default - however plugins for PHPstan and PsalmPHP are available to be used in e.g. PhpStorm and Visual Studio Code.

I’m just picking up the (incomplete) example from the beginning and will comment on that.

/**
 * @template T of object
 * @param class-string<T> $className
 * @param list<mixed> $constructorArguments
 * @return T
 */
public static function makeInstance(
  string $className,
  mixed ...$constructorArguments
): object {
}
  • added native types to function signature, thus it is
    • public static function makeInstance(string $className, mixed ...$constructorArguments): object instead of
    • public static function makeInstance($className, ...$constructorArguments)
    • → which makes the native @param and @return tags superfluous
  • use those native tags to provide additional(!) details for code analyzers
    • @template is not defined in classic PHPdoc → fine
    • if @param class-string $className is not understood by IDEs, there’s the fallback value in the function signature (string $className, ...) → fine
    • if @return T (and implicitly @template T) is not understood by IDEs, there’s the fallback value in the function signature ...): object → fine

Can you elaborate on why the example is incomplete?

Concerning your version of makeInstance WITH type declarations which doesn’t reflect the current state in the core, I agree completely. If tools can consume type declarations, I don’t have any problem with phpstan or psalm specific syntax in regular annotations.

But what about the cases where annotations still are the only source of truth?

Hey, I didn’t have time to dig into this before doing a statement but now I did. I have the phpstan plugin of jetbrains installed but all issues I experienced and wrote down in this thread are made with that plugin installed. So I’m honestly curious if I miss any other plugin that fixes the issues.

As for vscode I can confirm that there is a brilliant plugin that supports phpstan syntax.