Skip to content

Commit

Permalink
Squiz/ObjectInstantiation: bug fix - account for null coalesce
Browse files Browse the repository at this point in the history
The `Squiz.Objects.ObjectInstantiation` did not take null coalesce into account.

It also was potentially a little too lenient for ternaries as an inline then or inline else token before the `new` keyword would be considered an assignment, while it was never checked that the result of the ternary was actually assigned to something.

While still not 100%, this commit improves the sniffs to:
1. Allow for null coalesce assignments `??=`.
2. For ternary tokens + null coalesce `??`: verify if the result of either of these is actually "assigned" to something (or preceded by one of the other "allowed" tokens).

This should reduce the number of both false positives as well as false negatives, though there is still the potential for some false negatives - see the test on line 32 of the test case file.
This could be further reduced in a future iteration on this sniff, but for now, this fix can be considered a significant step forward.

Includes unit tests.

Fixes 3333
  • Loading branch information
jrfnl committed May 11, 2021
1 parent fde816c commit 790cf16
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 13 deletions.
38 changes: 27 additions & 11 deletions src/Standards/Squiz/Sniffs/Objects/ObjectInstantiationSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,37 @@ public function process(File $phpcsFile, $stackPtr)
$prev = $phpcsFile->findPrevious($allowedTokens, ($stackPtr - 1), null, true);

$allowedTokens = [
T_EQUAL => true,
T_DOUBLE_ARROW => true,
T_FN_ARROW => true,
T_MATCH_ARROW => true,
T_THROW => true,
T_RETURN => true,
T_INLINE_THEN => true,
T_INLINE_ELSE => true,
T_EQUAL => T_EQUAL,
T_COALESCE_EQUAL => T_COALESCE_EQUAL,
T_DOUBLE_ARROW => T_DOUBLE_ARROW,
T_FN_ARROW => T_FN_ARROW,
T_MATCH_ARROW => T_MATCH_ARROW,
T_THROW => T_THROW,
T_RETURN => T_RETURN,
];

if (isset($allowedTokens[$tokens[$prev]['code']]) === false) {
$error = 'New objects must be assigned to a variable';
$phpcsFile->addError($error, $stackPtr, 'NotAssigned');
if (isset($allowedTokens[$tokens[$prev]['code']]) === true) {
return;
}

$ternaryLikeTokens = [
T_COALESCE => true,
T_INLINE_THEN => true,
T_INLINE_ELSE => true,
];

// For ternary like tokens, walk a little further back to see if it is preceded by
// one of the allowed tokens (within the same statement).
if (isset($ternaryLikeTokens[$tokens[$prev]['code']]) === true) {
$hasAllowedBefore = $phpcsFile->findPrevious($allowedTokens, ($prev - 1), null, false, null, true);
if ($hasAllowedBefore !== false) {
return;
}
}

$error = 'New objects must be assigned to a variable';
$phpcsFile->addError($error, $stackPtr, 'NotAssigned');

}//end process()


Expand Down
18 changes: 18 additions & 0 deletions src/Standards/Squiz/Tests/Objects/ObjectInstantiationUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,24 @@ function returnMatch() {
}
}

// Issue 3333.
$time2 ??= new \DateTime();
$time3 = $time1 ?? new \DateTime();
$time3 = $time1 ?? $time2 ?? new \DateTime();

function_call($time1 ?? new \DateTime());
$return = function_call($time1 ?? new \DateTime()); // False negative depending on interpretation of the sniff.

function returnViaTernary() {
return ($y == false ) ? ($x === true ? new Foo : new Bar) : new FooBar;
}

function nonAssignmentTernary() {
if (($x ? new Foo() : new Bar) instanceof FooBar) {
// Do something.
}
}

// Intentional parse error. This must be the last test in the file.
function new
?>
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ class ObjectInstantiationUnitTest extends AbstractSniffUnitTest
public function getErrorList()
{
return [
5 => 1,
8 => 1,
5 => 1,
8 => 1,
31 => 1,
39 => 2,
];

}//end getErrorList()
Expand Down

0 comments on commit 790cf16

Please sign in to comment.