Merge pull request #1100 from WordPress-Coding-Standards/feature/assi…

…gnment-in-condition-handle-ternaries

AssignmentInCondition: also check ternary conditions + warning for non variable assignment

WordPress/Sniffs/CodeAnalysis/AssignmentInConditionSniff.php

@@ -17,7 +17,7 @@
  *
  * This is a typical code smell and more often than not a comparison was intended.
  *
- * Note: this sniff does not detect variable assignments in the conditional part of ternaries!
+ * Note: this sniff does not detect variable assignments in ternaries without parentheses!
  *
  * @package WPCS\WordPressCodingStandards
  *
@@ -64,6 +64,7 @@ public function register() {
 		$starters                       = Tokens::$booleanOperators;
 		$starters[ T_SEMICOLON ]        = T_SEMICOLON;
 		$starters[ T_OPEN_PARENTHESIS ] = T_OPEN_PARENTHESIS;
+		$starters[ T_INLINE_ELSE ]      = T_INLINE_ELSE;
 
 		$this->condition_start_tokens = $starters;
 
@@ -74,6 +75,7 @@ public function register() {
 			T_SWITCH,
 			T_CASE,
 			T_WHILE,
+			T_INLINE_THEN,
 		);
 
 	}//end register()
@@ -119,6 +121,29 @@ public function process_token( $stackPtr ) {
 			$opener = $stackPtr;
 			$closer = $token['scope_opener'];
 
+		} elseif ( T_INLINE_THEN === $token['code'] ) {
+			// Check if the condition for the ternary is bracketed.
+			$prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
+			if ( false === $prev ) {
+				// Shouldn't happen, but in that case we don't have anything to examine anyway.
+				return;
+			}
+
+			if ( T_CLOSE_PARENTHESIS === $this->tokens[ $prev ]['code'] ) {
+				if ( ! isset( $this->tokens[ $prev ]['parenthesis_opener'] ) ) {
+					return;
+				}
+
+				$opener = $this->tokens[ $prev ]['parenthesis_opener'];
+				$closer = $prev;
+			} elseif ( isset( $token['nested_parenthesis'] ) ) {
+				end( $token['nested_parenthesis'] );
+				$opener = key( $token['nested_parenthesis'] );
+				$closer = $stackPtr;
+			} else {
+				// No parenthesis found, can't determine where the conditional part of the ternary starts.
+				return;
+			}
 		} else {
 			if ( isset( $token['parenthesis_opener'], $token['parenthesis_closer'] ) === false ) {
 				return;
@@ -173,6 +198,12 @@ public function process_token( $stackPtr ) {
 					$hasAssignment,
 					'Found'
 				);
+			} else {
+				$this->phpcsFile->addWarning(
+					'Assignment found within a condition. Did you mean to do a comparison?',
+					$hasAssignment,
+					'NonVariableAssignmentFound'
+				);
 			}
 
 			$startPos = $hasAssignment;

WordPress/Tests/CodeAnalysis/AssignmentInConditionUnitTest.inc

@@ -24,24 +24,6 @@ do {
 } while ( $sample == false );
 
 while ( $sample === false ) {}
-while (list($field_name, $file_names) = each($formfiles)) {}
-
- // Silly, but not an assignment.
-if (123 = $a) {}
-if (strtolower($b) = $b) {}
-if (array( 1 => 'a', 2 => 'b' ) = $b) {}
-
-if (SOME_CONSTANT = 123) {
-} else if(self::SOME_CONSTANT -= 10) {}
-
-if ( $a() = 123 ) {
-} else if ( $b->something() = 123 ) {
-} elseif ( $c::something() = 123 ) {}
-
-switch ( true ) {
-	case 'something' = $sample:
-		break;
-}
 
 // Assignments in condition.
 if ($a = 123) {
@@ -92,3 +74,44 @@ while ( $sample = false ) {}
 
 if ($a = 123) :
 endif;
+
+// Non-variable assignment found.
+if (123 = $a) {}
+if (strtolower($b) = $b) {}
+if (array( 1 => 'a', 2 => 'b' ) = $b) {}
+
+if (SOME_CONSTANT = 123) {
+} else if(self::SOME_CONSTANT -= 10) {}
+
+if ( $a() = 123 ) {
+} else if ( $b->something() = 123 ) {
+} elseif ( $c::something() = 123 ) {}
+
+switch ( true ) {
+	case 'something' = $sample:
+		break;
+}
+
+while (list($field_name, $file_names) = each($formfiles)) {}
+
+/*
+ * Ternaries should also be checked, but can only be reliably done when in parenthesis.
+ */
+
+// OK.
+$mode = ( $a == 'something' ) ? 'on' : 'off';
+$mode = ( $a == 'on' ? 'true' : $a == 'off' ? 't' : 'f' );
+
+// Bad.
+$mode = ( $a = 'on' ) ? 'on' : 'off';
+$mode = ( $a = 'on' ) ?: 'off';
+$mode = ( $a = 'on' ) ? 'true' : ( $a = 'off' ) ? 't' : 'f'; // Bad x 2.
+$mode = ( $a = 'on' ? 'on' : 'off' );
+$mode = ( $a = 'on' ?: 'off' );
+$mode = ( $a = 'on' ? 'true' : ( $a = 'off' ? 't' : 'f' ) ); // Bad x 2.
+$mode = ( $a = 'on' ? 'true' : $a = 'off' ? 't' : 'f' ); // Bad x 3. The first ? triggers 1, the second (correctly) 2.
+
+// Currently not checked.
+$mode = $a = 'on' ? 'on' : 'off';
+$mode = $a = 'on' ?: 'off';
+$mode = $a = 'on' ? 'true' : $a = 'off' ? 't' : 'f';

WordPress/Tests/CodeAnalysis/AssignmentInConditionUnitTest.php

@@ -37,39 +37,56 @@ public function getErrorList() {
 	 */
 	public function getWarningList() {
 		return array(
-			47 => 1,
-			48 => 1,
-			49 => 1,
-			50 => 1,
-			51 => 1,
-			52 => 1,
-			53 => 1,
-			54 => 1,
-			55 => 1,
-			56 => 1,
-			57 => 1,
-			58 => 1,
-			59 => 1,
-			60 => 1,
-			61 => 1,
-			62 => 2,
-			64 => 1,
-			65 => 1,
-			68 => 1,
-			69 => 1,
-			70 => 1,
-			71 => 1,
-			72 => 1,
-			73 => 1,
-			74 => 1,
-			76 => 1,
-			78 => 1,
-			81 => 2,
-			85 => 1,
-			86 => 2,
-			89 => 1,
-			91 => 1,
-			93 => 1,
+			29  => 1,
+			30  => 1,
+			31  => 1,
+			32  => 1,
+			33  => 1,
+			34  => 1,
+			35  => 1,
+			36  => 1,
+			37  => 1,
+			38  => 1,
+			39  => 1,
+			40  => 1,
+			41  => 1,
+			42  => 1,
+			43  => 1,
+			44  => 2,
+			46  => 1,
+			47  => 1,
+			50  => 1,
+			51  => 1,
+			52  => 1,
+			53  => 1,
+			54  => 1,
+			55  => 1,
+			56  => 1,
+			58  => 1,
+			60  => 1,
+			63  => 2,
+			67  => 1,
+			68  => 2,
+			71  => 1,
+			73  => 1,
+			75  => 1,
+			79  => 1,
+			80  => 1,
+			81  => 1,
+			83  => 1,
+			84  => 1,
+			86  => 1,
+			87  => 1,
+			88  => 1,
+			91  => 1,
+			95  => 1,
+			106 => 1,
+			107 => 1,
+			108 => 2,
+			109 => 1,
+			110 => 1,
+			111 => 2,
+			112 => 3,
 		);
 
 	}