Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve handling of disable/enable/ignore directives
Browse files Browse the repository at this point in the history
The current method, listing codes to disable and a list of exceptions to
that list, still has trouble with some cases. For example, disabling a
standard, re-enabling a category within that standard, then ignoring or
disabling a sniff within that category cannot be handled. We'd need a
list of exceptions to the exceptions, and possibly a list of exceptions
to that list too, and figuring out how to keep those lists up to date as
new directives are encountered could prove to be confusing.

Since the standard→category→sniff→code hierarchy is supposed to be
thought of as a tree, let's store the ignore list that way instead.
Manipulating the branches of the tree is straightforward no matter what
directives are encountered.

In this implementation I've favored speed over space: there are cases
where we could prune a subtree that would evaluate to "ignore" or "don't
ignore" for any possible input, but detecting that doesn't seem worth
the time when it's not likely there will be so many enable or disable
directives that the wasted space will be a problem.

Fixes PHPCSStandards#111
anomiex committed Dec 4, 2023
1 parent c248a92 commit ade94ce
Showing 5 changed files with 525 additions and 98 deletions.
31 changes: 4 additions & 27 deletions src/Files/File.php
Original file line number Diff line number Diff line change
@@ -858,7 +858,7 @@ public function addFixableWarning(
protected function addMessage($error, $message, $line, $column, $code, $data, $severity, $fixable)
{
// Check if this line is ignoring all message codes.
if (isset($this->tokenizer->ignoredLines[$line]['.all']) === true) {
if (isset($this->tokenizer->ignoredLines[$line]) === true && $this->tokenizer->ignoredLines[$line]->isAll() === true) {
return false;
}

@@ -888,32 +888,9 @@ protected function addMessage($error, $message, $line, $column, $code, $data, $s
];
}//end if

if (isset($this->tokenizer->ignoredLines[$line]) === true) {
// Check if this line is ignoring this specific message.
$ignored = false;
foreach ($checkCodes as $checkCode) {
if (isset($this->tokenizer->ignoredLines[$line][$checkCode]) === true) {
$ignored = true;
break;
}
}

// If it is ignored, make sure it's not whitelisted.
if ($ignored === true
&& isset($this->tokenizer->ignoredLines[$line]['.except']) === true
) {
foreach ($checkCodes as $checkCode) {
if (isset($this->tokenizer->ignoredLines[$line]['.except'][$checkCode]) === true) {
$ignored = false;
break;
}
}
}

if ($ignored === true) {
return false;
}
}//end if
if (isset($this->tokenizer->ignoredLines[$line]) === true && $this->tokenizer->ignoredLines[$line]->check($sniffCode) === true) {
return false;
}

$includeAll = true;
if ($this->configCache['cache'] === false
107 changes: 36 additions & 71 deletions src/Tokenizers/Tokenizer.php
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@

use PHP_CodeSniffer\Exceptions\TokenizerException;
use PHP_CodeSniffer\Util;
use PHP_CodeSniffer\Util\IgnoreList;

abstract class Tokenizer
{
@@ -173,6 +174,7 @@ private function createPositionMap()
$lineNumber = 1;
$eolLen = strlen($this->eolChar);
$ignoring = null;
$ignoreAll = IgnoreList::ignoringAll();
$inTests = defined('PHP_CODESNIFFER_IN_TESTS');

$checkEncoding = false;
@@ -277,15 +279,15 @@ private function createPositionMap()
if ($ignoring === null
&& strpos($commentText, '@codingStandardsIgnoreStart') !== false
) {
$ignoring = ['.all' => true];
$ignoring = $ignoreAll;
if ($ownLine === true) {
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoring;
}
} else if ($ignoring !== null
&& strpos($commentText, '@codingStandardsIgnoreEnd') !== false
) {
if ($ownLine === true) {
$this->ignoredLines[$this->tokens[$i]['line']] = ['.all' => true];
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreAll;
} else {
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoring;
}
@@ -294,7 +296,7 @@ private function createPositionMap()
} else if ($ignoring === null
&& strpos($commentText, '@codingStandardsIgnoreLine') !== false
) {
$ignoring = ['.all' => true];
$ignoring = $ignoreAll;
if ($ownLine === true) {
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoring;
$this->ignoredLines[($this->tokens[$i]['line'] + 1)] = $ignoring;
@@ -393,7 +395,7 @@ private function createPositionMap()
if (substr($commentTextLower, 0, 9) === 'phpcs:set') {
// Ignore standards for complete lines that change sniff settings.
if ($lineHasOtherTokens === false) {
$this->ignoredLines[$this->tokens[$i]['line']] = ['.all' => true];
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreAll;
}

// Need to maintain case here, to get the correct sniff code.
@@ -416,42 +418,28 @@ private function createPositionMap()
} else if (substr($commentTextLower, 0, 13) === 'phpcs:disable') {
if ($lineHasOtherContent === false) {
// Completely ignore the comment line.
$this->ignoredLines[$this->tokens[$i]['line']] = ['.all' => true];
}

if ($ignoring === null) {
$ignoring = [];
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreAll;
}

$disabledSniffs = [];

$additionalText = substr($commentText, 14);
if (empty($additionalText) === true) {
$ignoring = ['.all' => true];
$ignoring = $ignoreAll;
} else {
if ($ignoring === null) {
$ignoring = IgnoreList::ignoringNone();
} else {
$ignoring = clone $ignoring;
}

$parts = explode(',', $additionalText);
foreach ($parts as $sniffCode) {
$sniffCode = trim($sniffCode);
$disabledSniffs[$sniffCode] = true;
$ignoring[$sniffCode] = true;

// This newly disabled sniff might be disabling an existing
// enabled exception that we are tracking.
if (isset($ignoring['.except']) === true) {
foreach (array_keys($ignoring['.except']) as $ignoredSniffCode) {
if ($ignoredSniffCode === $sniffCode
|| strpos($ignoredSniffCode, $sniffCode.'.') === 0
) {
unset($ignoring['.except'][$ignoredSniffCode]);
}
}

if (empty($ignoring['.except']) === true) {
unset($ignoring['.except']);
}
}
}//end foreach
}//end if
$ignoring->set($sniffCode, true);
}
}

$this->tokens[$i]['code'] = T_PHPCS_DISABLE;
$this->tokens[$i]['type'] = 'T_PHPCS_DISABLE';
@@ -464,49 +452,22 @@ private function createPositionMap()
if (empty($additionalText) === true) {
$ignoring = null;
} else {
$parts = explode(',', $additionalText);
$ignoring = clone $ignoring;
$parts = explode(',', $additionalText);
foreach ($parts as $sniffCode) {
$sniffCode = trim($sniffCode);
$enabledSniffs[$sniffCode] = true;
$ignoring->set($sniffCode, false);
}

// This new enabled sniff might remove previously disabled
// sniffs if it is actually a standard or category of sniffs.
foreach (array_keys($ignoring) as $ignoredSniffCode) {
if ($ignoredSniffCode === $sniffCode
|| strpos($ignoredSniffCode, $sniffCode.'.') === 0
) {
unset($ignoring[$ignoredSniffCode]);
}
}

// This new enabled sniff might be able to clear up
// previously enabled sniffs if it is actually a standard or
// category of sniffs.
if (isset($ignoring['.except']) === true) {
foreach (array_keys($ignoring['.except']) as $ignoredSniffCode) {
if ($ignoredSniffCode === $sniffCode
|| strpos($ignoredSniffCode, $sniffCode.'.') === 0
) {
unset($ignoring['.except'][$ignoredSniffCode]);
}
}
}
}//end foreach

if (empty($ignoring) === true) {
if ($ignoring->isEmpty() === true) {
$ignoring = null;
} else {
if (isset($ignoring['.except']) === true) {
$ignoring['.except'] += $enabledSniffs;
} else {
$ignoring['.except'] = $enabledSniffs;
}
}
}//end if
}

if ($lineHasOtherContent === false) {
// Completely ignore the comment line.
$this->ignoredLines[$this->tokens[$i]['line']] = ['.all' => true];
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreAll;
} else {
// The comment is on the same line as the code it is ignoring,
// so respect the new ignore rules.
@@ -523,31 +484,35 @@ private function createPositionMap()

$additionalText = substr($commentText, 13);
if (empty($additionalText) === true) {
$ignoreRules = ['.all' => true];
$ignoreRules = ['.all' => true];
$lineIgnoring = $ignoreAll;
} else {
$parts = explode(',', $additionalText);
if ($ignoring === null) {
$lineIgnoring = IgnoreList::ignoringNone();
} else {
$lineIgnoring = clone $ignoring;
}

foreach ($parts as $sniffCode) {
$ignoreRules[trim($sniffCode)] = true;
$lineIgnoring->set($sniffCode, true);
}
}

$this->tokens[$i]['code'] = T_PHPCS_IGNORE;
$this->tokens[$i]['type'] = 'T_PHPCS_IGNORE';
$this->tokens[$i]['sniffCodes'] = $ignoreRules;

if ($ignoring !== null) {
$ignoreRules += $ignoring;
}

if ($lineHasOtherContent === false) {
// Completely ignore the comment line, and set the following
// line to include the ignore rules we've set.
$this->ignoredLines[$this->tokens[$i]['line']] = ['.all' => true];
$this->ignoredLines[($this->tokens[$i]['line'] + 1)] = $ignoreRules;
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreAll;
$this->ignoredLines[($this->tokens[$i]['line'] + 1)] = $lineIgnoring;
} else {
// The comment is on the same line as the code it is ignoring,
// so respect the ignore rules it set.
$this->ignoredLines[$this->tokens[$i]['line']] = $ignoreRules;
$this->ignoredLines[$this->tokens[$i]['line']] = $lineIgnoring;
}
}//end if
}//end if
168 changes: 168 additions & 0 deletions src/Util/IgnoreList.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
<?php
/**
* Class to manage a list of sniffs to ignore.
*
* @author Brad Jorsch <brad.jorsch@automattic.com>
* @copyright 2023 Brad Jorsch
* @license https://github.com/squizlabs/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
*/

namespace PHP_CodeSniffer\Util;

class IgnoreList
{

/**
* Ignore data.
*
* Data is a tree, standard → category → sniff → code.
* Each level may be a boolean indicating that everything underneath the branch is or is not ignored, or
* may have a `.default' key indicating the default status for any branches not in the tree.
*
* @var array|boolean
*/
private $data = [ '.default' => false ];


/**
* Get an instance set to ignore nothing.
*
* @return static
*/
public static function ignoringNone()
{
return new static();

}//end ignoringNone()


/**
* Get an instance set to ignore everything.
*
* @return static
*/
public static function ignoringAll()
{
$ret = new static();
$ret->data['.default'] = true;
return $ret;

}//end ignoringAll()


/**
* Check whether a sniff code is ignored.
*
* @param string $code Partial or complete sniff code.
*
* @return bool
*/
public function check($code)
{
$data = $this->data;
$ret = $data['.default'];
foreach (explode('.', $code) as $part) {
if (isset($data[$part]) === false) {
break;
}

$data = $data[$part];
if (is_bool($data) === true) {
$ret = $data;
break;
}

if (isset($data['.default']) === true) {
$ret = $data['.default'];
}
}

return $ret;

}//end check()


/**
* Set the ignore status for a sniff.
*
* @param string $code Partial or complete sniff code.
* @param bool $ignore Whether the specified sniff should be ignored.
*
* @return this
*/
public function set($code, $ignore)
{
$data = &$this->data;
$parts = explode('.', $code);
while (count($parts) > 1) {
$part = array_shift($parts);
if (isset($data[$part]) === false) {
$data[$part] = [];
} else if (is_bool($data[$part]) === true) {
$data[$part] = [ '.default' => $data[$part] ];
}

$data = &$data[$part];
}

$part = array_shift($parts);
$data[$part] = (bool) $ignore;

return $this;

}//end set()


/**
* Check if the list is empty.
*
* @return bool
*/
public function isEmpty()
{
$arrs = [ $this->data ];
while ($arrs !== []) {
$arr = array_pop($arrs);
foreach ($arr as $v) {
if ($v === true) {
return false;
}

if (is_array($v) === true) {
$arrs[] = $v;
}
}
}

return true;

}//end isEmpty()


/**
* Check if the list ignores everything.
*
* @return bool
*/
public function isAll()
{
$arrs = [ $this->data ];
while ($arrs !== []) {
$arr = array_pop($arrs);
foreach ($arr as $v) {
if ($v === false) {
return false;
}

if (is_array($v) === true) {
$arrs[] = $v;
}
}
}

return true;

}//end isAll()


}//end class
64 changes: 64 additions & 0 deletions tests/Core/ErrorSuppressionTest.php
Original file line number Diff line number Diff line change
@@ -1046,6 +1046,70 @@ public function dataEnableSelected()
'expectedErrors' => 1,
'expectedWarnings' => 2,
],
'disable: two sniffs; enable: both sniffs; ignore: one of those sniffs (#3889)' => [
'code' => '
// phpcs:disable Generic.PHP.LowerCaseConstant
// phpcs:disable Generic.Commenting.Todo
//TODO: write some code
$var = TRUE;
// phpcs:enable Generic.Commenting.Todo
// phpcs:enable Generic.PHP.LowerCaseConstant
$var = FALSE; // phpcs:ignore Generic.PHP.LowerCaseConstant
',
'expectedErrors' => 0,
'expectedWarnings' => 0,
],
'disable: two sniffs; enable: one sniff; ignore: enabled sniff' => [
'code' => '
// phpcs:disable Generic.PHP.LowerCaseConstant
// phpcs:disable Generic.Commenting.Todo
//TODO: write some code
$var = TRUE;
// phpcs:enable Generic.PHP.LowerCaseConstant
$var = FALSE; // phpcs:ignore Generic.PHP.LowerCaseConstant
',
'expectedErrors' => 0,
'expectedWarnings' => 0,
],
'disable: two sniffs; enable: one sniff; ignore: category' => [
'code' => '
// phpcs:disable Generic.PHP.LowerCaseConstant
// phpcs:disable Generic.Commenting.Todo
//TODO: write some code
$var = TRUE;
// phpcs:enable Generic.PHP.LowerCaseConstant
$var = FALSE; // phpcs:ignore Generic.PHP
',
'expectedErrors' => 0,
'expectedWarnings' => 0,
],
'disable: two sniffs; enable: category; ignore: sniff in category' => [
'code' => '
// phpcs:disable Generic.PHP.LowerCaseConstant
// phpcs:disable Generic.Commenting.Todo
//TODO: write some code
$var = TRUE;
// phpcs:enable Generic.PHP
$var = FALSE; // phpcs:ignore Generic.PHP.LowerCaseConstant
',
'expectedErrors' => 0,
'expectedWarnings' => 0,
],
'disable: standard; enable: category in standard; disable: sniff in category' => [
'code' => '
// phpcs:disable Generic
// phpcs:enable Generic.PHP
// phpcs:disable Generic.PHP.LowerCaseConstant
//TODO: write some code
$var = TRUE;
',
'expectedErrors' => 0,
'expectedWarnings' => 0,
],
];

}//end dataEnableSelected()
253 changes: 253 additions & 0 deletions tests/Core/IgnoreListTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
<?php
/**
* Tests for the IgnoreList class.
*
* @author Brad Jorsch <brad.jorsch@automattic.com>
* @copyright 2023 Brad Jorsch
* @license https://github.com/squizlabs/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
*/

namespace PHP_CodeSniffer\Tests\Core;

use PHP_CodeSniffer\Util\IgnoreList;
use PHPUnit\Framework\TestCase;

class IgnoreListTest extends TestCase
{


/**
* Test ignoringNone() works.
*
* @covers PHP_CodeSniffer\Util\IgnoreList::ignoringNone
* @return void
*/
public function testIgnoringNoneWorks()
{
$ignoreList = IgnoreList::ignoringNone();
$this->assertInstanceOf(IgnoreList::class, $ignoreList);
$this->assertFalse($ignoreList->check('Anything'));

}//end testIgnoringNoneWorks()


/**
* Test ignoringAll() works.
*
* @covers PHP_CodeSniffer\Util\IgnoreList::ignoringAll
* @return void
*/
public function testIgnoringAllWorks()
{
$ignoreList = IgnoreList::ignoringAll();
$this->assertInstanceOf(IgnoreList::class, $ignoreList);
$this->assertTrue($ignoreList->check('Anything'));

}//end testIgnoringAllWorks()


/**
* Test isEmpty() and isAll().
*
* @param IgnoreList $ignoreList IgnoreList to test.
* @param bool $expectEmpty Expected return value from isEmpty().
* @param bool $expectAll Expected return value from isAll().
*
* @return void
*
* @dataProvider dataIsEmptyAndAll
* @covers PHP_CodeSniffer\Util\IgnoreList::isEmpty
* @covers PHP_CodeSniffer\Util\IgnoreList::isAll
*/
public function testIsEmptyAndAll($ignoreList, $expectEmpty, $expectAll)
{
$this->assertSame($expectEmpty, $ignoreList->isEmpty());
$this->assertSame($expectAll, $ignoreList->isAll());

}//end testIsEmptyAndAll()


/**
* Data provider.
*
* @see testIsEmptyAndAll()
*
* @return array
*/
public function dataIsEmptyAndAll()
{
return [
'fresh list' => [
new IgnoreList(),
true,
false,
],
'list from ignoringNone' => [
IgnoreList::ignoringNone(),
true,
false,
],
'list from ignoringAll' => [
IgnoreList::ignoringAll(),
false,
true,
],
'list from ignoringNone, something set to false' => [
IgnoreList::ignoringNone()->set('Foo.Bar', false),
true,
false,
],
'list from ignoringNone, something set to true' => [
IgnoreList::ignoringNone()->set('Foo.Bar', true),
false,
false,
],
'list from ignoringAll, something set to false' => [
IgnoreList::ignoringAll()->set('Foo.Bar', false),
false,
false,
],
'list from ignoringAll, something set to true' => [
IgnoreList::ignoringAll()->set('Foo.Bar', true),
false,
true,
],
'list from ignoringNone, something set to true then overridden' => [
IgnoreList::ignoringNone()->set('Foo.Bar', true)->set('Foo', false),
true,
false,
],
'list from ignoringAll, something set to false then overridden' => [
IgnoreList::ignoringAll()->set('Foo.Bar', false)->set('Foo', true),
false,
true,
],
];

}//end dataIsEmptyAndAll()


/**
* Test check() and set().
*
* @param array $toSet Associative array of $code => $ignore to pass to set().
* @param array $toCheck Associative array of $code => $expect to pass to check().
*
* @return void
*
* @dataProvider dataCheckAndSet
* @covers PHP_CodeSniffer\Util\IgnoreList::check
* @covers PHP_CodeSniffer\Util\IgnoreList::set
*/
public function testCheckAndSet($toSet, $toCheck)
{
$ignoreList = new IgnoreList();
foreach ($toSet as $code => $ignore) {
$this->assertSame($ignoreList, $ignoreList->set($code, $ignore));
}

foreach ($toCheck as $code => $expect) {
$this->assertSame($expect, $ignoreList->check($code));
}

}//end testCheckAndSet()


/**
* Data provider.
*
* @see testCheckAndSet()
*
* @return array
*/
public function dataCheckAndSet()
{
return [
'set a code' => [
['Standard.Category.Sniff.Code' => true],
[
'Standard.Category.Sniff.Code' => true,
'Standard.Category.Sniff.OtherCode' => false,
'Standard.Category.OtherSniff.Code' => false,
'Standard.OtherCategory.Sniff.Code' => false,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'set a sniff' => [
['Standard.Category.Sniff' => true],
[
'Standard.Category.Sniff.Code' => true,
'Standard.Category.Sniff.OtherCode' => true,
'Standard.Category.OtherSniff.Code' => false,
'Standard.OtherCategory.Sniff.Code' => false,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'set a category' => [
['Standard.Category' => true],
[
'Standard.Category.Sniff.Code' => true,
'Standard.Category.Sniff.OtherCode' => true,
'Standard.Category.OtherSniff.Code' => true,
'Standard.OtherCategory.Sniff.Code' => false,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'set a standard' => [
['Standard' => true],
[
'Standard.Category.Sniff.Code' => true,
'Standard.Category.Sniff.OtherCode' => true,
'Standard.Category.OtherSniff.Code' => true,
'Standard.OtherCategory.Sniff.Code' => true,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'set a standard, unignore a sniff in it' => [
[
'Standard' => true,
'Standard.Category.Sniff' => false,
],
[
'Standard.Category.Sniff.Code' => false,
'Standard.Category.Sniff.OtherCode' => false,
'Standard.Category.OtherSniff.Code' => true,
'Standard.OtherCategory.Sniff.Code' => true,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'set a standard, unignore a category in it, ignore a sniff in that' => [
[
'Standard' => true,
'Standard.Category' => false,
'Standard.Category.Sniff' => true,
],
[
'Standard.Category.Sniff.Code' => true,
'Standard.Category.Sniff.OtherCode' => true,
'Standard.Category.OtherSniff.Code' => false,
'Standard.OtherCategory.Sniff.Code' => true,
'OtherStandard.Category.Sniff.Code' => false,
],
],
'ignore some sniffs, then override some of those by unignoring the whole category' => [
[
'Standard.Category1.Sniff1' => true,
'Standard.Category1.Sniff2' => true,
'Standard.Category2.Sniff1' => true,
'Standard.Category2.Sniff2' => true,
'Standard.Category1' => false,
],
[
'Standard.Category1.Sniff1' => false,
'Standard.Category1.Sniff2' => false,
'Standard.Category2.Sniff1' => true,
'Standard.Category2.Sniff2' => true,
],
],
];

}//end dataCheckAndSet()


}//end class

0 comments on commit ade94ce

Please sign in to comment.