Skip to content

Commit 790cf16

Browse files
committed
Squiz/ObjectInstantiation: bug fix - account for null coalesce
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
1 parent fde816c commit 790cf16

File tree

3 files changed

+49
-13
lines changed

3 files changed

+49
-13
lines changed

src/Standards/Squiz/Sniffs/Objects/ObjectInstantiationSniff.php

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,21 +48,37 @@ public function process(File $phpcsFile, $stackPtr)
4848
$prev = $phpcsFile->findPrevious($allowedTokens, ($stackPtr - 1), null, true);
4949

5050
$allowedTokens = [
51-
T_EQUAL => true,
52-
T_DOUBLE_ARROW => true,
53-
T_FN_ARROW => true,
54-
T_MATCH_ARROW => true,
55-
T_THROW => true,
56-
T_RETURN => true,
57-
T_INLINE_THEN => true,
58-
T_INLINE_ELSE => true,
51+
T_EQUAL => T_EQUAL,
52+
T_COALESCE_EQUAL => T_COALESCE_EQUAL,
53+
T_DOUBLE_ARROW => T_DOUBLE_ARROW,
54+
T_FN_ARROW => T_FN_ARROW,
55+
T_MATCH_ARROW => T_MATCH_ARROW,
56+
T_THROW => T_THROW,
57+
T_RETURN => T_RETURN,
5958
];
6059

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

64+
$ternaryLikeTokens = [
65+
T_COALESCE => true,
66+
T_INLINE_THEN => true,
67+
T_INLINE_ELSE => true,
68+
];
69+
70+
// For ternary like tokens, walk a little further back to see if it is preceded by
71+
// one of the allowed tokens (within the same statement).
72+
if (isset($ternaryLikeTokens[$tokens[$prev]['code']]) === true) {
73+
$hasAllowedBefore = $phpcsFile->findPrevious($allowedTokens, ($prev - 1), null, false, null, true);
74+
if ($hasAllowedBefore !== false) {
75+
return;
76+
}
77+
}
78+
79+
$error = 'New objects must be assigned to a variable';
80+
$phpcsFile->addError($error, $stackPtr, 'NotAssigned');
81+
6682
}//end process()
6783

6884

src/Standards/Squiz/Tests/Objects/ObjectInstantiationUnitTest.inc

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,24 @@ function returnMatch() {
2323
}
2424
}
2525

26+
// Issue 3333.
27+
$time2 ??= new \DateTime();
28+
$time3 = $time1 ?? new \DateTime();
29+
$time3 = $time1 ?? $time2 ?? new \DateTime();
30+
31+
function_call($time1 ?? new \DateTime());
32+
$return = function_call($time1 ?? new \DateTime()); // False negative depending on interpretation of the sniff.
33+
34+
function returnViaTernary() {
35+
return ($y == false ) ? ($x === true ? new Foo : new Bar) : new FooBar;
36+
}
37+
38+
function nonAssignmentTernary() {
39+
if (($x ? new Foo() : new Bar) instanceof FooBar) {
40+
// Do something.
41+
}
42+
}
43+
2644
// Intentional parse error. This must be the last test in the file.
2745
function new
2846
?>

src/Standards/Squiz/Tests/Objects/ObjectInstantiationUnitTest.php

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@ class ObjectInstantiationUnitTest extends AbstractSniffUnitTest
2626
public function getErrorList()
2727
{
2828
return [
29-
5 => 1,
30-
8 => 1,
29+
5 => 1,
30+
8 => 1,
31+
31 => 1,
32+
39 => 2,
3133
];
3234

3335
}//end getErrorList()

0 commit comments

Comments
 (0)