Errors for lines containing error suppression comments are always suppressed

[jrfnl]

The new error suppression system ignores all notices for lines which only contain the new PHPCS native whitelist comments // phpcs:.....

While this is great for, for instance, the LineLength sniff, it also means that, for instance, the SuperfluousWhitespace or DisallowTabIndent sniffs will not work for the lines containing these type of comments. Which means you still could end up with tab indentation even when space indentation is enforced.

Also, the suppression on all notices for lines which only contain the new PHPCS native whitelist comment, also prevents examining these comments.

I was already discussing for one project to not allow "blanket" comment, i.e. // phpcs:disable without a sniffcode would not be allowed to be used in that project.
Similarly, for that project, we would want to enforce, that all whitelist comments (except // phpcs:enable) have to be accompanied by a reason for whitelisting.

Suppressing all errors for these type of lines, would also prevent any automated checks for this.

Any thoughts ?

[gsherwood]

To clarify, this isn't new line ignore behaviour - these comments have always suppressed the line they are on and the following line. The changed behaviour is that if the comment is on a line with other code, it will only ignore that one line and not the following one. And the comments have their own tokens, so you can now listen for them more easily, but you could do this sort of checking with the old syntax as well if you really wanted to.

I just wanted to make that clear as there isn't an older behaviour that can be reverted back to, and the behaviour is intentional.

I don't think there are many options here. Those comments must have all errors suppressed so commenting sniffs (or anything really) don't start throwing errors for PHPCS instructions. Yes, specific sniffs can now detect those special tokens and exclude errors manually, but that's a lot of work by a lot of sniffs and a big BC break. Plus, PHPCS would need to decide on situations like if a phpcs:ignoreFile comment truly ignored the file or allows sniffs that want to ban the usage of phpcs:ignoreFile to run, and what happens between phpcs:disable and phpcs:enable comments. I know you want to ban that, but PHPCS can't assume that.

There are two workarounds, but neither are very good.

1. Report errors on a different line to the comment; maybe even on line 1 with a reference to the bad line in the error message. This will work most of the time, unless the line you pick is also being ignored.

2. Do a second phpcs run using the --ignore-annotations CLI argument and use the --sniffs argument (or a different ruleset maybe) to just inspect these types of comments. No errors will be suppressed, so you can get access to the full error list. This wont work during IDE inspections, but you could put it into a build system.

The second option is better, if you can live with developers not seeing errors until build time. Both could also be done - an annoying error on the wrong line during live coding and then a build step to ensure all checks are done properly.

Thinking about it a bit more, a build step would probably be a good idea anyway if you want to ensure a developer hasn't used annotations to ignore your annotation checks (like with ignoreFile).

[jrfnl]

I realize the behaviour is not new, but in certain cases - like for sniffs regarding superfluous whitespace, type of whitespace or indentation -, I would call it undesired.

For me, the big difference between the "old style" comments and the new style is their flexibility and ability to document.

I rarely used the old style comments and actively discouraged people from using them as they would generally ignore way too much - all sniffs, while most of the time only one sniff needed to be ignored -.
IMHO, it was better practice to exclude a particular file for one sniff from within the ruleset and document in the ruleset why, than resort to the old comments.

The new style comments with their modular ability to ignore/disable very select sniffs or even error codes and the ability to document why, however are completely different and I will actively favour them over exclusions from the ruleset now.

This in practice - at least for me - means that there will be a lot more suppression comments in code, which makes it all the more relevant for them to:

• use the correct type of indentation (tabs vs spaces)
• be indented correctly
• comply with other whitespace rules

That's aside from any specific requirements which a project may or may not have for those comments as described above.

A secondary build could work, but that would mean that all other code would have to comply 100% with the sniffs which would be included as otherwise the build would fail - so things like phpcs:ignoreFile would too easily break the secondary build.

Alternatively, a separate sniff would need to be written to check just the // phpcs: comments and just for what is needed, but that would need to partially copy the ScopeIndentation sniff in that case, which is a daunting task and - to be fair - quite undesired.

Hmm... not sure what to do now... I'm going to sleep on this one.

[jrfnl]

Quick question - just checking -, when --ignore-annotations is used, the // phpcs: comments are not tokenized with their own tokens, are they ? So that would mean that all comment tokens would need to be examined anew ?

[gsherwood]

when --ignore-annotations is used, the // phpcs: comments are not tokenized with their own tokens, are they ?

Looks like it, as an optimisation. If that was the only thing stopping a solution, they could still be tokenized without taking effect.

[jrfnl]

they could still be tokenized without taking effect.

I think, for consistency and predictability, it would be a good idea to tokenize files the same whether --ignore-annotations is used or not.

I noticed another issue with the tokenization of the new annotations while working on making the sniffs compatible - when used in a docblock and using the new @phpcs: annotation, only the prefix is tokenized as T_PHPCS_..., the rest of the line is tokenized as T_DOC_COMMENT_WHITESPACE and T_DOC_COMMENT_STRING.
If the annotation is not prefixed with an @, the whole line is tokenized as T_PHPCS_...

Example code to demonstrate:

<?php

/**
 * Something
 *
 * @param string $abc
 * @return bool
 *
 * @phpcs:ignore Standard.Category.Sniff -- for reasons.
 * phpcs:disable Standard.Category.Sniff -- for reasons.
 * @phpcs:ignoreFile Standard.Category.Sniff
 */
function abc( $abc ) {
	do_something();
}

Tokenizes as:

Process token 32 on line 9 [col:4;len:13;lvl:0;]: T_PHPCS_IGNORE => @phpcs:ignore
Process token 33 on line 9 [col:17;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 34 on line 9 [col:18;len:39;lvl:0;]: T_DOC_COMMENT_STRING => Standard.Category.Sniff -- for reasons.
Process token 35 on line 9 [col:57;len:0;lvl:0;]: T_DOC_COMMENT_WHITESPACE => \n
Process token 36 on line 10 [col:1;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 37 on line 10 [col:2;len:1;lvl:0;]: T_DOC_COMMENT_STAR => *
Process token 38 on line 10 [col:3;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 39 on line 10 [col:4;len:53;lvl:0;]: T_PHPCS_DISABLE => phpcs:disable Standard.Category.Sniff -- for reasons.
Process token 40 on line 10 [col:57;len:0;lvl:0;]: T_DOC_COMMENT_WHITESPACE => \n
Process token 41 on line 11 [col:1;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 42 on line 11 [col:2;len:1;lvl:0;]: T_DOC_COMMENT_STAR => *
Process token 43 on line 11 [col:3;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 44 on line 11 [col:4;len:17;lvl:0;]: T_PHPCS_IGNORE_FILE => @phpcs:ignoreFile
Process token 45 on line 11 [col:21;len:1;lvl:0;]: T_DOC_COMMENT_WHITESPACE =>
Process token 46 on line 11 [col:22;len:23;lvl:0;]: T_DOC_COMMENT_STRING => Standard.Category.Sniff
Process token 47 on line 11 [col:45;len:0;lvl:0;]: T_DOC_COMMENT_WHITESPACE => \n

A further inconsistency there is that when used in a // or /* */ style comment, the T_PHPCS_... token will always contain the new line at the end of the line.
While, when used in a docblock, it will not and the new line is tokenized separately as T_DOC_COMMENT_WHITESPACE.

As a side-note: I'm currently presuming that all docblock related sniffs use the $commentTokens or $emptyTokens array to walk the comment, however if not and there are sniff which only look for the T_DOC_COMMENT tokens, the PHPCS annotation tokens "breaking up" the docblock could also give issues in sniffs.

[jrfnl]

☝️ the tokenizer inconsistency with docblocks in combination with @phpcs: syntax will also cause these comments to become blanket ignore/disables.
Adding a Sniff, Standard etc, no longer has any effect!

[jrfnl]

Found yet another issue.

If a PHPCS annotation token is part of a changeset, for instance for fixing indentation of arrays or docblocks , the line containing the PHPCS annotation will not be fixed.

Now this behaviour is properly not new, but like I said before, I expect these annotations to be used a lot more than the old style annotations, so I expect that you'll start to see more bug reports about this soon enough.

Proposal:
* If a changeset contains a fix to a PHPCS token as part of a larger group of fixes, apply them.
* If it is a singular change, i.e. not part of a changeset, ignore them, like the error would be ignored.

[Edit]: it might be better to not ignore errors for the whole line when the annotation is the only non-whitespace token on the line, but just to ignore errors for the actual PHPCS token.
That way indentation issues and such could still be reported & fixed like normal.

[jrfnl]

@gsherwood Just checking - have you had a chance to read through the above three comments and formed an opinion on this ?

The issue with docblocks and @phpcs: annotations becoming blanket ignore/disables is one I really believe should be fixed before the 3.2.3 release.

Clarification about the direction you will be taking regarding the inconsistencies will help me to update/fix the last four sniffs which I know are having trouble with the new PHPCS annotations.
(Not claiming completeness, but those four are the last ones based on a search for T_COMMENT used in sniffs).

[gsherwood]

have you had a chance to read through the above three comments and formed an opinion on this ?

Sorry, no. Probably another 24 hours before I can get back to PHPCS work.

[gsherwood]

The issue with docblocks and @phpcs: annotations becoming blanket ignore/disables is one I really believe should be fixed before the 3.2.3 release.

I've fixed that one. Thanks a lot for finding that before it got released. I'll take a look at your other comments now.

[gsherwood]

If a PHPCS annotation token is part of a changeset, for instance for fixing indentation of arrays or docblocks , the line containing the PHPCS annotation will not be fixed.

That's pretty tricky. That comment is entirely ignored to ensure it doesn't trigger things like commenting sniffs that try to reformat it, or any other custom sniffs really. If I just let PHPCS start reporting fixable errors during fixing, the comment will be changed in ways that PHPCS never complained about, which could be a fairly serious problem.

it might be better to not ignore errors for the whole line when the annotation is the only non-whitespace token on the line, but just to ignore errors for the actual PHPCS token.

I can probably do that without breaking too much, but it wont help indentation issues because the error is reported on the comment token itself, so it will still be ignored.

A sniff is still able to change that comment token during fixing. It just isn't able to kick off fixing code if it reports an error on the line the comment is on. So if a sniff went through and fixed the indentation of an entire IF statement because the indent of the IF keyword was wrong, it will still fix the comment indent. For example:

<?php
if ($foo) {
if ($bar) {
    //phpcs:ignore Foo -- fake code to ensure closing brace is fix
}
}

Gets fixed as:

<?php
if ($foo) {
    if ($bar) {
        //phpcs:ignore Foo -- fake code to ensure closing brace is fix
    }
}

So it's not like PHPCS doesn't allow those tokens to be fixed. It's just that it never reports errors with those lines and so PHPCBF shouldn't be implementing fixes for those hidden errors.

I'm not sure how to change that behaviour without breaking things.

[gsherwood]

Just revisiting this as it's stuck at the moment.

I still can't figure out an east way to achieve what you're after without affecting the normal behaviour of PHPCS.

If a developer decides to write a comment to ignore a line, and they choose to indent that line incorrectly (for example) and not conform to the rules you've required for those annotation (e.g., always put a comment after the annotation) I can't see how PHPCS can figure out that errors reported on that line are actually valid. It was a developer-level directive.

So, where to go from here?

Ultimately, we need the errors reported, so PHPCS can't drop them. If it can't drop them, it needs to flag them as being ignored from normal reporting.

Then we need a way to disable this flag and make them available for normal reporting again. The errors exist, so we know they have error codes. Maybe we can add a ruleset.xml config that tells PHPCS to selectively surface these errors again. I'm thinking something that can be applied to any <rule> tag so it works at the standard/category/sniff/message level.

If this sounds like something that could work, we could work a bit on the XML syntax for re-surfacing the errors.

Thoughts?

[jrfnl]

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#11