Psalm: @psalm-suppress should not be limited to docblocks

Created on 12 Dec 2019 · 13Comments · Source: vimeo/psalm

Currently psalm-suppress is only allowed in a doc block.
This causes issues when using other validation tools like phpcs,... as doc blocks are not valid in specific places (and usually also require a description.

It would be better if psalm-suppress would also be valid in a normal comment like:

/* @psalm-suppress RedundantCondition */
if ( empty( $foo ) ) {
    $foo = 'bar';
}

and also as:

// @psalm-suppress RedundantCondition
if ( empty( $foo ) ) {
    $foo = 'bar';
}

This would also make the behavior more consistent to what phpcs:ignore has

Source

kkmuffme

All 13 comments

This would mean that Psalm would have to parse standard comments for @psalm-suppress annotations which would slow things down a _little_.

What do others think?

muglug on 12 Dec 2019

I would say those other validation tools should be configured to ignore the tags they don't understand.

weirdan on 12 Dec 2019

👍1

@weirdan that's not the problem, is it? The issue is that /** is a standardised PHPDoc doc-block comment and psalm is (mis)using it for suppressing errors, while it mostly is just 1 single line comment, where other comment forms are more appropriate

kkmuffme on 12 Dec 2019

Can you provide examples where you think it's wrong and elaborate a bit on why you think it's wrong?

weirdan on 13 Dec 2019

Actually all cases where the suppress is NOT within a function, var, class or file doc, a normal comment would be better fitted.

/** @psalm-suppress ... */
if ( empty( $var ) ) {

/** @psalm-suppress ... */
echo $foo;

in both cases, the preferred comment would be // @psalm-suppress ... by most developers/code styles

kkmuffme on 13 Dec 2019

👎2

The issue is that /** is a standardised PHPDoc doc-block comment and psalm is (mis)using it for suppressing errors, while it mostly is just 1 single line comment

Inline docblocks like /** @psalm-suppress ... */ are perfectly fine and I would say psalm is making proper use of them rather than misusing.

the preferred comment would be // @psalm-suppress ... by most developers/code styles

That might be your personal preference but it would be wrong to claim it is for "most developers/code styles" :slightly_smiling_face:.

ADmad on 14 Dec 2019

👍1

This would mean that Psalm would have to parse standard comments for @psalm-suppress annotations which would slow things down a little.

@muglug If you do end up supporting psalm annotations as regular comments too then IMHO it should be an opt-in feature so that it doesn't slow down things for others.

ADmad on 14 Dec 2019

in both cases, the preferred comment would be // @psalm-suppress ... by most developers/code styles

Why? You mentioned standardization as one of the reasons, yet PSR-5 specifically mentions DocComments on non-structural elements as out of scope of the standard (see the definition of Structural Element). Argument can be made that it's still a draft, but it's the only attempt to standardize PHP docblocks I'm aware of.

I also find the distinction between comment forms useful: /** */ are machine-readable, while // and /* */ are for humans.

weirdan on 14 Dec 2019

👍1

@ADmad

That might be your personal preference but it would be wrong to claim it is for "most developers/code styles" slightly_smiling_face.

except that literally >50% of the internet use this standard, which is "most". e.g. WordPress, Drupal, Magento, Joomla, Facebook (API),...
Besides the fact that other PHP code analysis tools (phpcs,...) that are much more commonly used than psalm support it as well

If you do end up supporting psalm annotations as regular comments too then IMHO it should be an opt-in feature so that it doesn't slow down things for others.

Parsing 10 million lines of code for a comment like // @psalm-suppress literally takes 2 seconds on the slowest EC2 instance using PHP. If this second matters for you, you probably shouldn't be using PHP in the first place as it's obviously much too slow for your use case.

@weirdan your link states this is for docblocks, but normal comments should NOT be docblocks. e.g. psalm-suppress in a function declaration docblock makes perfect sense.
As docblocks should be used before STRUCTURAL ELEMENTS.
But my above examples are "Control Flow" statements rather than a "Structural Element", so this guide does not say docblocks should be used there.

I also find the distinction between comment forms useful: /* */ are machine-readable, while // and / */ are for humans.

Not true, because doc blocks always contain human readable data. e.g. a function declaration docblock has a short description of the function, a file doc. Even when using @psalm-suppress, do you really not leave a comment in the doc block why you are using psalm-suppress in this instance?

kkmuffme on 15 Dec 2019

If this second matters for you, you probably shouldn't be using PHP in the first place as it's obviously much too slow for your use case.

Thanks for your unwanted advice.

your link states this is for docblocks, but normal comments should NOT be docblocks. e.g. psalm-suppress in a function declaration docblock makes perfect sense.

But the @psalm-* tags are NOT normal comments. They are special annotations and hence using doc-blocks to denote they have a special meaning is the appropriate thing to do.

ADmad on 15 Dec 2019

Thanks for your unwanted advice.

You brought up the argument for speed though; just telling you that it doesn't matter since the impact is negligibly small.

But the @psalm-* tags are NOT normal comments. They are special annotations and hence using doc-blocks to denote they have a special meaning is the appropriate thing to do.

I totally get your point and understand why you want to use docblocks for psalm-suppress.
But this doesn't mean that others shouldn't be allowed to use psalm-suppress in normal comments*, since phpcs, etc. also support this.
It is also common in other languages, e.g. eslint for js also has it's ignore statements in normal comments and not in docblocks.

*As weirdan also pointed out above already, docblocks should be used for structural elements. But psalm-suppress can be used on flow statements either, where in most common convention docblocks aren't used.

kkmuffme on 15 Dec 2019

As docblocks should be used before STRUCTURAL ELEMENTS.
But my above examples are "Control Flow" statements rather than a "Structural Element", so this guide does not say docblocks should be used there.

As weirdan also pointed out above already, docblocks should be used for structural elements.

Perhaps you missed the point about DocComments on non-structural element being out of scope of the standard:

An example of use that falls beyond the scope of this Standard is to document the variable in a foreach explicitly; several IDEs use this information to assist their auto-completion functionality.

This Standard does not cover this specific instance, as a foreach statement is considered to be a "Control Flow" statement rather than a "Structural Element".
/** @var \Sqlite3 $sqlite */
foreach ($connections as $sqlite) {
   $sqlite->open('/my/database/path');
   <...>
}

In the same vein other uses of DocComments on non-structural elements like those you used with @psalm-suppress are not covered either.