Skip to content

Commit

Permalink
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 #3889
  • Loading branch information
@anomiex
anomiex committed Sep 21, 2023
1 parent 7566b4d commit 62ac505
Show file tree
Hide file tree
Showing 6 changed files with 529 additions and 98 deletions.
4 changes: 4 additions & 0 deletions package.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
<file baseinstalldir="" name="AbstractMethodUnitTest.php" role="test" />
<file baseinstalldir="" name="AllTests.php" role="test" />
<file baseinstalldir="" name="ErrorSuppressionTest.php" role="test" />
<file baseinstalldir="" name="IgnoreListTest.php" role="test" />
<file baseinstalldir="" name="IsCamelCapsTest.php" role="test" />
</dir>
<dir name="Standards">
Expand Down Expand Up @@ -2115,6 +2116,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
<dir name="Util">
<file baseinstalldir="PHP/CodeSniffer" name="Cache.php" role="php" />
<file baseinstalldir="PHP/CodeSniffer" name="Common.php" role="php" />
<file baseinstalldir="PHP/CodeSniffer" name="IgnoreList.php" role="php" />
<file baseinstalldir="PHP/CodeSniffer" name="Standards.php" role="php" />
<file baseinstalldir="PHP/CodeSniffer" name="Timing.php" role="php" />
<file baseinstalldir="PHP/CodeSniffer" name="Tokens.php" role="php" />
Expand Down Expand Up @@ -2165,6 +2167,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
<install as="CodeSniffer/Core/AllTests.php" name="tests/Core/AllTests.php" />
<install as="CodeSniffer/Core/IsCamelCapsTest.php" name="tests/Core/IsCamelCapsTest.php" />
<install as="CodeSniffer/Core/ErrorSuppressionTest.php" name="tests/Core/ErrorSuppressionTest.php" />
<install as="CodeSniffer/Core/IgnoreListTest.php" name="tests/Core/IgnoreListTest.php" />
<install as="CodeSniffer/Core/Autoloader/DetermineLoadedClassTest.php" name="tests/Core/Autoloader/DetermineLoadedClassTest.php" />
<install as="CodeSniffer/Core/Autoloader/TestFiles/A.inc" name="tests/Core/Autoloader/TestFiles/A.inc" />
<install as="CodeSniffer/Core/Autoloader/TestFiles/B.inc" name="tests/Core/Autoloader/TestFiles/B.inc" />
Expand Down Expand Up @@ -2289,6 +2292,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
<install as="CodeSniffer/Core/AllTests.php" name="tests/Core/AllTests.php" />
<install as="CodeSniffer/Core/IsCamelCapsTest.php" name="tests/Core/IsCamelCapsTest.php" />
<install as="CodeSniffer/Core/ErrorSuppressionTest.php" name="tests/Core/ErrorSuppressionTest.php" />
<install as="CodeSniffer/Core/IgnoreListTest.php" name="tests/Core/IgnoreListTest.php" />
<install as="CodeSniffer/Core/Autoloader/DetermineLoadedClassTest.php" name="tests/Core/Autoloader/DetermineLoadedClassTest.php" />
<install as="CodeSniffer/Core/Autoloader/TestFiles/A.inc" name="tests/Core/Autoloader/TestFiles/A.inc" />
<install as="CodeSniffer/Core/Autoloader/TestFiles/B.inc" name="tests/Core/Autoloader/TestFiles/B.inc" />
Expand Down
31 changes: 4 additions & 27 deletions src/Files/File.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,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;
}

Expand Down Expand Up @@ -872,32 +872,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
Expand Down
107 changes: 36 additions & 71 deletions src/Tokenizers/Tokenizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

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

abstract class Tokenizer
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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';
Expand All @@ -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.
Expand All @@ -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
Expand Down

0 comments on commit 62ac505

Please sign in to comment.