Squiz.Commenting.PostStatementComment should allow trailing annotations from popular PHP tools

[rodrigoprimo]

Is your feature request related to a problem?

Currently, the Squiz.Commenting.PostStatementComment sniff makes it impossible to use trailing annotations like PHPUnit's // @codeCoverageIgnore. This happens because the sniff forbids inline comments after statements (with some exceptions).

Describe the solution you'd like

The Squiz.Commenting.PostStatementComment already makes some exceptions and allows inline comments after statements in a few cases. Most notably, it allows PHPCS's // phpcs:ignore trailing annotation. I believe the sniff should also allow trailing annotations used by other popular PHP tools.

Here is a non-exhaustive list of trailing annotations that the sniff should allow:

• PHPUnit's // @codeCoverageIgnore (https://docs.phpunit.de/en/11.2/code-coverage.html#ignoring-code-blocks)
• PHPStan's // @phpstan-ignore and // @phpstan-ignore-line (https://phpstan.org/user-guide/ignoring-errors)

If this change is implemented, the code examples below, which currently are flagged as errors by the sniff, will stop being flagged:

$a = 1; // @codeCoverageIgnore
$b = 2; // @phpstan-ignore-line
$c = 3; // @phpstan-ignore variable.undefined

Open questions:

• Are there other trailing annotations used by popular PHP tools that the sniff should allow as well?
• PHPUnit doesn't support anything after // @codeCoverageIgnore while PHPStan requires an error identifier and, optionally, a comment after // @phpstan-ignore. I think that the sniff doesn't need to conform to the exact rules used by each tool, and just checking that a given comment starts with the annotation supported by the tool is enough. So, for example, $a = 1; // @codeCoverageIgnore some comment is not supported by PHPUnit, but would not be flagged by the sniff.

Additional context (optional)

This was originally discussed with @jrfnl in #525 (review).

If the change proposed here is accepted, this will allow PHPCS itself to use // @codeCoverageIgnore in its codebase to make PHPUnit ignore lines of code that cannot be covered by tests when generating the code coverage report.

• I have read the Contribution Guidelines and this is not a support question.
• I intend to create a pull request to implement this feature.

[jrfnl]

Thanks for opening this issue @rodrigoprimo.

As you may have guessed, I'm in favour of the proposed change.

Having said that, I don't think the sniff should blindly ignore trailing comments which are tool related annotations. I think these should still be flagged, but flagged with a separate error code - maybe AnnotationFound ? -, which will allow ruleset maintainers to selectively exclude the flagging of trailing annotations from their ruleset.

This also minimizes the BC-break/change in behaviour for the sniff.

Open questions:

• Are there other trailing annotations used by popular PHP tools that the sniff should allow as well?

Maybe the sniff shouldn't look for specific annotations, but for any trailing comment which starts with an @ sign + tag, i.e. '^// @[a-z0-9-]+'i ?

That would allow it to also handle things like trailing // @todo comments and annotations of other tools, like Psalm, without the need to maintain an "allow list" with patterns to allow.

• PHPUnit doesn't support anything after // @codeCoverageIgnore while PHPStan requires an error identifier and, optionally, a comment after // @phpstan-ignore. I think that the sniff doesn't need to conform to the exact rules used by each tool, and just checking that a given comment starts with the annotation supported by the tool is enough. So, for example, $a = 1; // @codeCoverageIgnore some comment is not supported by PHPUnit, but would not be flagged by the sniff.

IMO whether the annotation format is supported by the tool it is intended for, is 100% out of scope. The format of the annotation/comment is not the concern of this sniff. This sniff only concerns itself with trailing comments.

Think of it this way: the sniff also doesn't check whether a non-annotation comment uses proper punctuation, so why would it check the format of tooling annotations ?

[jrfnl]

P.S.: by the looks of it, there is a work-around which could be applied at this time already and that is to use a trailing comment starting with #. Not sure if the other tooling would recognize their annotations correctly still, but that's a different matter.

Might be good to open a separate bug report for this as trailing comments are trailing comments, whether they start with // or with #....

[rodrigoprimo]

Having said that, I don't think the sniff should blindly ignore trailing comments which are tool related annotations. I think these should still be flagged, but flagged with a separate error code - maybe AnnotationFound ? -, which will allow ruleset maintainers to selectively exclude the flagging of trailing annotations from their ruleset.

I don't have a strong preference, and your suggestion works for me. I can prepare a PR implementing it. There is just one more thing that I want to check with you before proceeding. See below.

Maybe the sniff shouldn't look for specific annotations, but for any trailing comment which starts with an @ sign + tag, i.e. '^// @[a-z0-9-]+'i ?

I believe the hyphen is not necessary in the regex unless we expect an annotation that starts with a hyphen? I guess just '|^// @[a-z0-9]+|i' is enough, or am I missing something?

I'm not sure if there is a standard definition of what are valid characters for an annotation. I don't know if an annotation can start with or contain an underscore, for example. If there is no definition, maybe we should consider a comment to be an annotation if it starts with @?

Think of it this way: the sniff also doesn't check whether a non-annotation comment uses proper punctuation, so why would it check the format of tooling annotations ?

👍

P.S.: by the looks of it, there is a work-around which could be applied at this time already and that is to use a trailing comment starting with #. Not sure if the other tooling would recognize their annotations correctly still, but that's a different matter.

# @codeCoverageIgnore is not recognized by PHPUnit, and we couldn't use it anyway in the PHPCS code base as its coding standard includes Squiz.Commenting.InlineComment.WrongStyle.

Might be good to open a separate bug report for this as trailing comments are trailing comments, whether they start with // or with #....

My guess is that Squiz.Commenting.PostStatementComment doesn't check for Perl-style comments because of Squiz.Commenting.InlineComment.WrongStyle, but the sniffs should be independent of one another. I agree that Squiz.Commenting.PostStatementComment should flag Perl-style comments.

Here is the issue: #562

[jrfnl]

Maybe the sniff shouldn't look for specific annotations, but for any trailing comment which starts with an @ sign + tag, i.e. '^// @[a-z0-9-]+'i ?

I believe the hyphen is not necessary in the regex unless we expect an annotation that starts with a hyphen? I guess just '|^// @[a-z0-9]+|i' is enough, or am I missing something?

I'm not sure if there is a standard definition of what are valid characters for an annotation. I don't know if an annotation can start with or contain an underscore, for example. If there is no definition, maybe we should consider a comment to be an annotation if it starts with @?

@rodrigoprimo I think the - is necessary, but better yet, maybe the same regex base as is used for tags in doc comments should be used ? This basically comes down to an @ with at least one character behind it, any character at all, as long as it is not whitespace.

PHP_CodeSniffer/src/Tokenizers/Comment.php

Line 202 in 9168f53

preg_match('/@[^\s]+/', $string, $matches, 0, $start);

[jrfnl]

Oh and for the record: There may or may not be a space between the comment marker and the @ (confirmed via a code search of a larger set of projects, including confirmation that both PHPCS as well as PHPUnit support both), so the space should be regarded as optional.