Skip to content

Avoid incompatibility between Generic and PSR2 standards for function call indentation #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Avoid incompatibility between Generic and PSR2 standards for function call indentation #38

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ff2dd3
Avoid incompatibility between two sniffs
fredden Sep 6, 2023
e98cbb5
Fix bug when tabs are used for indentation
fredden Jan 17, 2024
8ce76fa
Merge remote-tracking branch 'upstream/master' into whitespace-incomp…
fredden Jan 17, 2024
ddd7f72
Merge remote-tracking branch 'upstream/master' into whitespace-incomp…
fredden Apr 30, 2024
810f747
Avoid conflicts with Generic.WhiteSpace.ScopeIndent
fredden Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.3.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,18 @@ if ($foo) {
$this->foo()
->bar()
->baz();

// https://github.com/squizlabs/PHP_CodeSniffer/issues/2078#issuecomment-401641650
// See also PSR2.Methods.FunctionCallSignature
$repository->foo()
->bar(
function () {
return true;
}
);
$repository->foo()
->bar(
function () {
return true;
}
);
15 changes: 15 additions & 0 deletions src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.3.inc.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,18 @@ if ($foo) {
$this->foo()
->bar()
->baz();

// https://github.com/squizlabs/PHP_CodeSniffer/issues/2078#issuecomment-401641650
// See also PSR2.Methods.FunctionCallSignature
$repository->foo()
->bar(
function () {
return true;
}
);
$repository->foo()
->bar(
function () {
return true;
}
);
3 changes: 3 additions & 0 deletions src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ public function getErrorList($testFile='ScopeIndentUnitTest.inc')
6 => 1,
7 => 1,
10 => 1,
40 => 1,
41 => 1,
42 => 1,
];
}

Expand Down
109 changes: 83 additions & 26 deletions src/Standards/PEAR/Sniffs/Functions/FunctionCallSignatureSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,30 +376,16 @@ public function processMultiLineCall(File $phpcsFile, $stackPtr, $openBracket, $
// at a tab stop. Without this, the function will be indented a further
// $indent spaces to the right.
$functionIndent = (int) (floor($foundFunctionIndent / $this->indent) * $this->indent);
$adjustment = 0;
$adjustment = ($functionIndent - $foundFunctionIndent);

if ($foundFunctionIndent !== $functionIndent) {
$error = 'Opening statement of multi-line function call not indented correctly; expected %s spaces but found %s';
$data = [
$this->complainOpenStatementWrongIndent(
$phpcsFile,
$first,
$tokens,
$functionIndent,
$foundFunctionIndent,
];

$fix = $phpcsFile->addFixableError($error, $first, 'OpeningIndent', $data);
if ($fix === true) {
// Set adjustment for use later to determine whether argument indentation is correct when fixing.
$adjustment = ($functionIndent - $foundFunctionIndent);

$padding = str_repeat(' ', $functionIndent);
if ($foundFunctionIndent === 0) {
$phpcsFile->fixer->addContentBefore($first, $padding);
} else if ($tokens[$first]['code'] === T_INLINE_HTML) {
$newContent = $padding.ltrim($tokens[$first]['content']);
$phpcsFile->fixer->replaceToken($first, $newContent);
} else {
$phpcsFile->fixer->replaceToken(($first - 1), $padding);
}
}
$foundFunctionIndent
);
}//end if

$next = $phpcsFile->findNext(Tokens::$emptyTokens, ($openBracket + 1), null, true);
Expand Down Expand Up @@ -465,7 +451,7 @@ public function processMultiLineCall(File $phpcsFile, $stackPtr, $openBracket, $
$i--;
}

for ($i; $i < $closeBracket; $i++) {
for (; $i < $closeBracket; $i++) {
if ($i > $argStart && $i < $argEnd) {
$inArg = true;
} else {
Expand Down Expand Up @@ -545,10 +531,34 @@ public function processMultiLineCall(File $phpcsFile, $stackPtr, $openBracket, $
$foundIndent = $tokens[$i]['length'];
}

if ($foundIndent < $expectedIndent
|| ($inArg === false
&& $expectedIndent !== $foundIndent)
) {
$indentCorrect = true;

if ($foundIndent < $expectedIndent) {
$indentCorrect = false;
} else if ($inArg === false && $expectedIndent !== $foundIndent) {
$indentCorrect = false;

// It is permitted to indent chains further than one tab stop to
// align vertically with the previous method call.
if ($i === ($closeBracket - 1)) {
if ($foundIndent === $foundFunctionIndent) {
// This is the closing paren; it lines up vertically with the opening paren.
$indentCorrect = true;
}
} else {
if ($foundIndent === ($tokens[$openBracket]['column'] - 1)) {
// This is a parameter; it lines up vertically with the opening paren.
$indentCorrect = true;
}

if ($foundIndent === ($foundFunctionIndent + ($this->indent))) {
// This is a parameter; it is indented one more step than the function call around it.
$indentCorrect = true;
}
}
}//end if

if ($indentCorrect === false) {
$error = 'Multi-line function call not indented correctly; expected %s spaces but found %s';
$data = [
$expectedIndent,
Expand Down Expand Up @@ -631,4 +641,51 @@ public function processMultiLineCall(File $phpcsFile, $stackPtr, $openBracket, $
}//end processMultiLineCall()


/**
* Add a complaint (and auto-fix) if the function indent is 'wrong'.
*
* @param File $phpcsFile The file being scanned.
* @param int $first Pointer to the first empty token on this line.
* @param array $tokens The stack of tokens that make up the file.
* @param int $functionIndent The expected indent for this function definition.
* @param int $foundFunctionIndent The actual indent for this function definition.
*
* @return void
*/
protected function complainOpenStatementWrongIndent(
$phpcsFile,
$first,
$tokens,
$functionIndent,
$foundFunctionIndent
) {
if ($foundFunctionIndent === $functionIndent) {
return;
}

$error = 'Opening statement of multi-line function call not indented correctly; expected %s spaces but found %s';
$data = [
$functionIndent,
$foundFunctionIndent,
];

$fix = $phpcsFile->addFixableError($error, $first, 'OpeningIndent', $data);
if ($fix !== true) {
return;
}

$padding = str_repeat(' ', $functionIndent);

if ($foundFunctionIndent === 0) {
$phpcsFile->fixer->addContentBefore($first, $padding);
} else if ($tokens[$first]['code'] === T_INLINE_HTML) {
$newContent = $padding.ltrim($tokens[$first]['content']);
$phpcsFile->fixer->replaceToken($first, $newContent);
} else {
$phpcsFile->fixer->replaceToken(($first - 1), $padding);
}

}//end complainOpenStatementWrongIndent()


}//end class
11 changes: 2 additions & 9 deletions src/Standards/PEAR/Tests/Functions/FunctionCallSignatureUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public function getErrorList($testFile='FunctionCallSignatureUnitTest.inc')
82 => 1,
93 => 1,
100 => 1,
106 => 2,
106 => 1,
119 => 1,
120 => 1,
129 => 1,
Expand Down Expand Up @@ -98,15 +98,9 @@ public function getErrorList($testFile='FunctionCallSignatureUnitTest.inc')
346 => 2,
353 => 1,
354 => 1,
355 => 2,
355 => 1,
377 => 1,
378 => 1,
379 => 1,
380 => 1,
385 => 1,
386 => 1,
387 => 1,
388 => 1,
393 => 1,
394 => 1,
395 => 1,
Expand Down Expand Up @@ -135,7 +129,6 @@ public function getErrorList($testFile='FunctionCallSignatureUnitTest.inc')
567 => 1,
568 => 1,
573 => 1,
574 => 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these errors no longer being thrown, gives me the impression you are breaking the PEAR sniff behaviour in favour of fixing something which affects only PSR2. That doesn't seem right....

What is the justification for this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone back through these changes and re-assessed each in turn. Below are my findings, grouped together by class / reasoning.

Lines 106 & 355

There were two errors being flagged for this line:

106 | ERROR | [x] Multi-line function call not indented correctly; expected 0 spaces but found 4 (PEAR.Functions.FunctionCallSignature.Indent)
106 | ERROR | [x] Closing parenthesis of a multi-line function call must be on a line by itself (PEAR.Functions.FunctionCallSignature.CloseBracketLine)

The first error is actually not correct (or is misleading). The parameter is correctly indented by 4 spaces. The closing parenthesis should be indented by zero spaces, but not on this line. Fixing the second error (which is still reported after the changes in this pull request) by moving the closing parenthesis to its own line makes this clear as the indents for the parameter and closing parenthesis are then checked/reported independently.

The error message when the line is incorrectly indented (eg with 5 spaces) is misleading as it reports "expected 0 spaces but found 5" whereas it should probably report "expected 4 spaces but found 5" for the parameter (and leave the closing parenthesis to report its own indentation anomalies when it's on its own line.). I'm not trying to fix that problem here; this is an improvement that could be made in the future.

Lines 378-380 & 386-388 (& 394-396)

This code snippet shows these errors:

377 | ERROR | [x] Opening statement of multi-line function call not indented correctly; expected 4 spaces but found 5 (PEAR.Functions.FunctionCallSignature.OpeningIndent)
378 | ERROR | [x] Multi-line function call not indented correctly; expected 9 spaces but found 8 (PEAR.Functions.FunctionCallSignature.Indent)
379 | ERROR | [x] Multi-line function call not indented correctly; expected 9 spaces but found 8 (PEAR.Functions.FunctionCallSignature.Indent)
380 | ERROR | [x] Multi-line function call not indented correctly; expected 5 spaces but found 4 (PEAR.Functions.FunctionCallSignature.Indent)

There is ambiguity in the sniff here in that it will accept any multiple of 4 spaces (including zero) on line 377. When line 377 is no longer being flagged as problematic, the following three lines are either correct (when 377 has four spaces) or off by some multiple of four (eg, when 377 has 12 leading spaces, the following three lines are each indented by 8 too few spaces).

The coding standard does say that "Parameters need to be indented 4 spaces compared to the level of the function call", so I can see why the following three lines are being reported as "wrong" by this sniff. However the same coding standard says to "Use an indent of 4 spaces", which makes me think that "expected 9 spaces" and "expected 5 spaces" are in violation of the coding standard.

Lines 394-396 don't show up in the delta for this pull request an error message is still shown for these lines, but the message is changing in this pull request. Previously the sniff would suggest an indent of 13/9 spaces, now the sniff suggests an indent of 12/8 spaces.

Line 574

This one is harder to justify. The sniff (now) accepts both 4 & 6 spaces indentation here as "correct." Should we be accepting only one of these numbers? I can't see any examples of "inline HTML" in the coding standard at https://pear.php.net/manual/en/standards.php

];

}//end getErrorList()
Expand Down
24 changes: 23 additions & 1 deletion src/Standards/PSR2/Sniffs/Methods/FunctionCallSignatureSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class FunctionCallSignatureSniff extends PEARFunctionCallSignatureSniff


/**
* Processes single-line calls.
* Determine if this is a multi-line function call.
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
Expand Down Expand Up @@ -76,4 +76,26 @@ public function isMultiLineCall(File $phpcsFile, $stackPtr, $openBracket, $token
}//end isMultiLineCall()


/**
* No-op; this rule does not apply to PSR-2.
*
* @param File $phpcsFile The file being scanned.
* @param int $first Pointer to the first empty token on this line.
* @param array $tokens The stack of tokens that make up the file.
* @param int $functionIndent The expected indent for this function definition.
* @param int $foundFunctionIndent The actual indent for this function definition.
*
* @return void
*/
protected function complainOpenStatementWrongIndent(
$phpcsFile,
$first,
$tokens,
$functionIndent,
$foundFunctionIndent
) {

}//end complainOpenStatementWrongIndent()


}//end class
15 changes: 15 additions & 0 deletions src/Standards/PSR2/Tests/Methods/FunctionCallSignatureUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,3 +265,18 @@ array_fill_keys(
), value: true,
);
// phpcs:set PSR2.Methods.FunctionCallSignature allowMultipleArguments false

// https://github.com/squizlabs/PHP_CodeSniffer/issues/2078#issuecomment-401641650
// This sniff should accept both of these styles. Generic.WhiteSpace.ScopeIndent can complain & fix this if enabled.
$repository->foo()
->bar(
function () {
return true;
}
);
$repository->foo()
->bar(
function () {
return true;
}
);
15 changes: 15 additions & 0 deletions src/Standards/PSR2/Tests/Methods/FunctionCallSignatureUnitTest.inc.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,3 +281,18 @@ array_fill_keys(
), value: true,
);
// phpcs:set PSR2.Methods.FunctionCallSignature allowMultipleArguments false

// https://github.com/squizlabs/PHP_CodeSniffer/issues/2078#issuecomment-401641650
// This sniff should accept both of these styles. Generic.WhiteSpace.ScopeIndent can complain & fix this if enabled.
$repository->foo()
->bar(
function () {
return true;
}
);
$repository->foo()
->bar(
function () {
return true;
}
);
4 changes: 2 additions & 2 deletions src/Standards/PSR2/Tests/Methods/FunctionCallSignatureUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ public function getErrorList()
187 => 1,
194 => 3,
199 => 1,
200 => 2,
200 => 1,
202 => 1,
203 => 1,
210 => 2,
211 => 1,
212 => 2,
212 => 1,
217 => 1,
218 => 1,
227 => 1,
Expand Down
3 changes: 0 additions & 3 deletions src/Standards/PSR2/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,6 @@
<rule ref="PSR2.Methods.FunctionCallSignature.SpaceAfterCloseBracket">
<severity>0</severity>
</rule>
<rule ref="PSR2.Methods.FunctionCallSignature.OpeningIndent">
<severity>0</severity>
</rule>

<!-- 5. Control Structures -->

Expand Down