Make NoEmptyIfDefinedPlugin check all node types except AST_DIM

This should be perfectly fine because the expression should never be unset. Bug: T349446 Change-Id: Icb9f1d1ff6eb678a70609b842b7898667aeb09e1
wikimedia · Oct 24, 2023 · fe43331 · fe43331
1 parent 679ac25
commit fe43331
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 27 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -2,7 +2,8 @@
 
 ## UNRELEASED
 * Add plugin to disallow use of `new Exception` (Daimona Eaytoy)
-* Do not emit MediaWikiNoEmptyIfDefined for properties of classes with the AllowDynamicProperties attribute (Daimona Eaytoy) 
+* Do not emit MediaWikiNoEmptyIfDefined for properties of classes with the AllowDynamicProperties attribute (Daimona Eaytoy)
+* Emit MediaWikiNoEmptyIfDefined for all node types except array element access (Daimona Eaytoy)
 
 ## 0.13.0 / 2023-09-08
 * Add plugin to forbid `empty()` on defined variables and properties (Daimona Eaytoy)

diff --git a/src/Plugin/NoEmptyIfDefinedVisitor.php b/src/Plugin/NoEmptyIfDefinedVisitor.php
@@ -13,17 +13,11 @@
 use Phan\Exception\NodeException;
 use Phan\Exception\UnanalyzableException;
 use Phan\PluginV3\PluginAwarePostAnalysisVisitor;
+use const ast\AST_DIM;
 use const ast\AST_PROP;
 use const ast\AST_STATIC_PROP;
-use const ast\AST_VAR;
 
 class NoEmptyIfDefinedVisitor extends PluginAwarePostAnalysisVisitor {
-	private const ANALYSED_EXPR_TYPES = [
-		AST_VAR,
-		AST_PROP,
-		AST_STATIC_PROP,
-	];
-
 	/**
 	 * @inheritDoc
 	 */
@@ -34,11 +28,24 @@ public function visitEmpty( Node $node ): void {
 		}
 		$expr = $node->children['expr'];
 
-		if ( !$expr instanceof Node || !in_array( $expr->kind, self::ANALYSED_EXPR_TYPES, true ) ) {
-			// Only check "simple" node types. In particular, we're skipping array index access because that's a
-			// lesser issue even if the array element is guaranteed to be set, and also, it's not possible to
-			// analyze it properly given the FLAG_IGNORE_UNDEF trickery below.
-			return;
+		if ( !$expr instanceof Node || !$this->exprIsPossiblyUndefined( $expr ) ) {
+			self::emitPluginIssue(
+				$this->code_base,
+				$this->context,
+				NoEmptyIfDefinedPlugin::ISSUE_TYPE,
+				// Links to https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP#empty()
+				'Found usage of {FUNCTIONLIKE} on expression {CODE} that appears to be always set. ' .
+				'{FUNCTIONLIKE} should only be used to suppress errors. See https://w.wiki/6paE',
+				[ 'empty()', ASTReverter::toShortString( $expr ), 'empty()' ]
+			);
+		}
+	}
+
+	private function exprIsPossiblyUndefined( Node $expr ): bool {
+		if ( $expr->kind === AST_DIM ) {
+			// Skip this case, because it's a lesser issue even if the array element is guaranteed to be set, and also,
+			// it's not possible to analyze it properly given the FLAG_IGNORE_UNDEF trickery below.
+			return true;
 		}
 
 		if ( $expr->kind === AST_PROP || $expr->kind === AST_STATIC_PROP ) {
@@ -51,11 +58,11 @@ public function visitEmpty( Node $node ): void {
 			} catch ( IssueException | NodeException | UnanalyzableException $_ ) {
 				// Bail out if the expr is a property that phan can't resolve. In this scenario the union type will
 				// be empty, but not possibly undefined, yet we shouldn't emit an issue.
-				return;
+				return true;
 			}
 			if ( $property->getClass( $this->code_base )->hasDynamicProperties( $this->code_base ) ) {
 				// stdClass or another class with dynamic properties. These are always possibly undefined.
-				return;
+				return true;
 			}
 		}
 
@@ -70,21 +77,11 @@ public function visitEmpty( Node $node ): void {
 			);
 		} catch ( IssueException $_ ) {
 			// Ignore, because we're removing FLAG_IGNORE_UNDEF in a hacky way.
-			return;
+			return true;
 		} finally {
 			$expr->flags |= $prevIgnoreUndefFlag;
 		}
 
-		if ( !$type->isPossiblyUndefined() ) {
-			self::emitPluginIssue(
-				$this->code_base,
-				$this->context,
-				NoEmptyIfDefinedPlugin::ISSUE_TYPE,
-				// Links to https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP#empty()
-				'Found usage of {FUNCTIONLIKE} on expression {CODE} that appears to be always set. ' .
-				'{FUNCTIONLIKE} should only be used to suppress errors. See https://w.wiki/6paE',
-				[ 'empty()', ASTReverter::toShortString( $expr ), 'empty()' ]
-			);
-		}
+		return $type->isPossiblyUndefined();
 	}
 }
diff --git a/tests/plugins/NoEmptyIfDefinedPlugin/functions/expectedResults.txt b/tests/plugins/NoEmptyIfDefinedPlugin/functions/expectedResults.txt
@@ -0,0 +1,3 @@
+plugins/NoEmptyIfDefinedPlugin/functions/test.php:18 MediaWikiNoEmptyIfDefined Found usage of empty() on expression mayReturnNull() that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/functions/test.php:23 MediaWikiNoEmptyIfDefined Found usage of empty() on expression $class->nonStaticMethod() that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/functions/test.php:26 MediaWikiNoEmptyIfDefined Found usage of empty() on expression TestNoEmptyOnMethodCalls::staticMethod() that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
diff --git a/tests/plugins/NoEmptyIfDefinedPlugin/functions/test.php b/tests/plugins/NoEmptyIfDefinedPlugin/functions/test.php
@@ -0,0 +1,29 @@
+<?php
+
+function mayReturnNull(): ?int {
+	return rand() ? 42 : null;
+}
+
+class TestNoEmptyOnMethodCalls {
+	public function nonStaticMethod(): ?int {
+		return rand() ? 42 : null;
+	}
+
+	public static function staticMethod(): ?int {
+		return rand() ? 42 : null;
+	}
+}
+
+function testFunctionCalls() {
+	if ( empty( mayReturnNull() ) ) {
+		echo "hello";
+	}
+
+	$class = new TestNoEmptyOnMethodCalls();
+	if ( empty( $class->nonStaticMethod() ) ) {
+		echo "hello";
+	}
+	if ( empty( TestNoEmptyOnMethodCalls::staticMethod() ) ) {
+		echo "hello";
+	}
+}
diff --git a/tests/plugins/NoEmptyIfDefinedPlugin/miscnodes/expectedResults.txt b/tests/plugins/NoEmptyIfDefinedPlugin/miscnodes/expectedResults.txt
@@ -0,0 +1,20 @@
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:6 MediaWikiNoEmptyIfDefined Found usage of empty() on expression 1 that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:7 MediaWikiNoEmptyIfDefined Found usage of empty() on expression (2 + 2) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:8 MediaWikiNoEmptyIfDefined Found usage of empty() on expression true that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:9 MediaWikiNoEmptyIfDefined Found usage of empty() on expression 1 that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:10 MediaWikiNoEmptyIfDefined Found usage of empty() on expression rand() that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:11 MediaWikiNoEmptyIfDefined Found usage of empty() on expression ($x = null) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:13 MediaWikiNoEmptyIfDefined Found usage of empty() on expression ($arr[42] = 42) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:14 MediaWikiNoEmptyIfDefined Found usage of empty() on expression ($y = 42) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:15 MediaWikiNoEmptyIfDefined Found usage of empty() on expression empty($z) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:16 MediaWikiNoEmptyIfDefined Found usage of empty() on expression (fn) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:17 MediaWikiNoEmptyIfDefined Found usage of empty() on expression (true && false) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:18 PhanUndeclaredVariable Variable $x1 is undeclared (Did you mean $x)
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:19 MediaWikiNoEmptyIfDefined Found usage of empty() on expression isset($x2) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:20 PhanUndeclaredVariable Variable $x3 is undeclared (Did you mean $x)
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:20 PhanUndeclaredVariable Variable $x4 is undeclared (Did you mean $x)
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:21 MediaWikiNoEmptyIfDefined Found usage of empty() on expression new stdClass() that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:22 PhanTypeInvalidUnaryOperandIncOrDec Invalid operator: unary operand of (expr)++ is null= (expected int or string or float)
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:22 PhanUndeclaredVariable Variable $doesNotExist is undeclared
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:24 MediaWikiNoEmptyIfDefined Found usage of empty() on expression (clone($obj)) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
+plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php:25 MediaWikiNoEmptyIfDefined Found usage of empty() on expression require($GLOBALS['some_file']) that appears to be always set. empty() should only be used to suppress errors. See https://w.wiki/6paE
diff --git a/tests/plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php b/tests/plugins/NoEmptyIfDefinedPlugin/miscnodes/test.php
@@ -0,0 +1,26 @@
+<?php
+
+// @phan-file-suppress PhanNoopEmpty
+
+function doTestMisc() {
+	empty( 1 );
+	empty( 2 + 2 );
+	empty( true );
+	empty( ( 1 ) );
+	empty( ( rand() ) );
+	empty( $x = null );
+	$arr = [];
+	empty( $arr[42] = 42 );
+	empty( $y = 42 );
+	empty( empty( $z ) );
+	empty( fn () => 42 );
+	empty( true && false );
+	empty( (int)$x1 ); // Redundant empty(), but not reported because an issue is emitted for the expr
+	empty( isset( $x2 ) );
+	empty( rand() ? $x3 : $x4 ); // Redundant empty(), but not reported because an issue is emitted for the expr
+	empty( new stdClass );
+	empty( $doesNotExist++ ); // Redundant empty(), but not reported because an issue is emitted for the expr
+	$obj = new stdClass;
+	empty( clone $obj );
+	empty( require $GLOBALS['some_file'] );
+}