Add plugin to disallow use of new Exception

The `Exception` base class should never be used directly, so add a
plugin to flag usages.

Bug: T338423
Change-Id: I1877ff36b69ccfe151b1637904f6be77ce7334d5

src/Plugin/NoBaseExceptionPlugin.php

@@ -0,0 +1,27 @@
+<?php
+
+declare( strict_types=1 );
+
+namespace MediaWikiPhanConfig\Plugin;
+
+use Phan\PluginV3;
+use Phan\PluginV3\PostAnalyzeNodeCapability;
+
+// HACK: Avoid redeclaring the class if phan `require`s this file multiple times (e.g., in tests, where
+// we reset the plugin list)
+if ( !class_exists( NoBaseExceptionPlugin::class ) ) {
+	class NoBaseExceptionPlugin extends PluginV3 implements PostAnalyzeNodeCapability {
+
+		public const ISSUE_TYPE = 'MediaWikiNoBaseException';
+
+		/**
+		 * @inheritDoc
+		 */
+		public static function getPostAnalyzeNodeVisitorClassName(): string {
+			return NoBaseExceptionVisitor::class;
+		}
+
+	}
+}
+
+return new NoBaseExceptionPlugin();

src/Plugin/NoBaseExceptionVisitor.php

@@ -0,0 +1,77 @@
+<?php
+
+declare( strict_types=1 );
+
+namespace MediaWikiPhanConfig\Plugin;
+
+use ast\Node;
+use Phan\AST\UnionTypeVisitor;
+use Phan\Exception\FQSENException;
+use Phan\Exception\IssueException;
+use Phan\Language\FQSEN\FullyQualifiedClassName;
+use Phan\PluginV3\PluginAwarePostAnalysisVisitor;
+use const ast\AST_NAME;
+
+class NoBaseExceptionVisitor extends PluginAwarePostAnalysisVisitor {
+	/**
+	 * @inheritDoc
+	 */
+	public function visitNew( Node $node ): void {
+		$classNode = $node->children['class'];
+		if ( !$classNode instanceof Node ) {
+			// Syntax error, bail out
+			return;
+		}
+		if ( $classNode->kind !== AST_NAME ) {
+			// A variable or another dynamic expression, skip for now.
+			// TODO We could try and handle this, at least in the easy cases (e.g., if the value of the variable
+			// can be inferred statically).
+			// Note that we can't just check the union type, because it could be `Exception` even if the class is not
+			// `Exception` itself (e.g., if the union type is inferred from a doc comment or typehint that only uses
+			// `Exception` as the ancestor type of all the potential values).
+			return;
+		}
+		if ( !isset( $classNode->children['name'] ) ) {
+			// Syntax error, bail out
+			return;
+		}
+
+		try {
+			$classNameType = UnionTypeVisitor::unionTypeFromClassNode(
+				$this->code_base,
+				$this->context,
+				$classNode
+			);
+		} catch ( IssueException | FQSENException $_ ) {
+			return;
+		}
+
+		if ( $classNameType->typeCount() !== 1 ) {
+			// Shouldn't happen
+			return;
+		}
+		$classType = $classNameType->getTypeSet()[0];
+		if ( !$classType->isObjectWithKnownFQSEN() ) {
+			// Syntax error or something.
+			return;
+		}
+		$classFQSEN = $classType->asFQSEN();
+		if ( !$classFQSEN instanceof FullyQualifiedClassName ) {
+			// Should not happen.
+			return;
+		}
+
+		$classFQSENStr = (string)$classFQSEN;
+		if ( $classFQSENStr === '\Exception' ) {
+			self::emitPluginIssue(
+				$this->code_base,
+				$this->context,
+				NoBaseExceptionPlugin::ISSUE_TYPE,
+				// Links to https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP#Exception_handling
+				'Instantiating {CLASS} directly is not allowed. Use SPL exceptions if the exception is ' .
+					'unchecked, or define a custom exception class otherwise. See https://w.wiki/6nur',
+				[ $classFQSENStr ]
+			);
+		}
+	}
+}

src/Plugin/NoEmptyIfDefinedPlugin.php

@@ -7,17 +7,21 @@
 use Phan\PluginV3;
 use Phan\PluginV3\PostAnalyzeNodeCapability;
 
-class NoEmptyIfDefinedPlugin extends PluginV3 implements PostAnalyzeNodeCapability {
+// HACK: Avoid redeclaring the class if phan `require`s this file multiple times (e.g., in tests, where
+// we reset the plugin list)
+if ( !class_exists( NoEmptyIfDefinedPlugin::class ) ) {
+	class NoEmptyIfDefinedPlugin extends PluginV3 implements PostAnalyzeNodeCapability {
 
-	public const ISSUE_TYPE = 'MediaWikiNoEmptyIfDefined';
+		public const ISSUE_TYPE = 'MediaWikiNoEmptyIfDefined';
 
-	/**
-	 * @inheritDoc
-	 */
-	public static function getPostAnalyzeNodeVisitorClassName(): string {
-		return NoEmptyIfDefinedVisitor::class;
-	}
+		/**
+		 * @inheritDoc
+		 */
+		public static function getPostAnalyzeNodeVisitorClassName(): string {
+			return NoEmptyIfDefinedVisitor::class;
+		}
 
+	}
 }
 
 return new NoEmptyIfDefinedPlugin();

src/base-config-functions.php

@@ -78,7 +78,8 @@ function setBaseOptions( string $curDir, ConfigBuilder $configBuilder ): void {
 			'AddNeverReturnTypePlugin',
 		] )
 		->addCustomPlugins( [
-			'NoEmptyIfDefinedPlugin'
+			'NoEmptyIfDefinedPlugin',
+			'NoBaseExceptionPlugin',
 		] );
 
 	if ( !defined( 'MSG_EOR' ) ) {

tests/PluginTest.php

@@ -1,8 +1,10 @@
 <?php
 
 use Phan\CLIBuilder;
+use Phan\Config;
 use Phan\Output\Printer\PlainTextPrinter;
 use Phan\Phan;
+use Phan\Plugin\ConfigPluginSet;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Console\Output\BufferedOutput;
 
@@ -51,6 +53,7 @@ private function runPhan( string $plugin, string $cfgFile, bool $usePolyfill ):
 			$this->markTestSkipped( 'This test requires PHP extension \'ast\' loaded' );
 		}
 
+		Config::reset();
 		$codeBase = require __DIR__ . '/../vendor/phan/phan/src/codebase.php';
 		$cliBuilder = new CLIBuilder();
 		$cliBuilder->setOption( 'project-root-directory', __DIR__ );
@@ -62,6 +65,11 @@ private function runPhan( string $plugin, string $cfgFile, bool $usePolyfill ):
 		}
 		$cli = $cliBuilder->build();
 
+		// Reset the plugin config so that things from previous tests do not persist.
+		// This is not handled by PHPUnit because the singleton is stored in a local static variable.
+		// And it can't be done earlier, because reset() recomputes the list of plugins, for which we need the
+		// config to be loaded.
+		ConfigPluginSet::reset();
 		$stream = new BufferedOutput();
 		$printer = new PlainTextPrinter();
 		$printer->configureOutput( $stream );

tests/plugins/NoBaseExceptionPlugin/basic/expectedResults.txt

@@ -0,0 +1,2 @@
+plugins/NoBaseExceptionPlugin/basic/test.php:15 MediaWikiNoBaseException Instantiating /Exception directly is not allowed. Use SPL exceptions if the exception is unchecked, or define a custom exception class otherwise. See https://w.wiki/6nur
+plugins/NoBaseExceptionPlugin/basic/test.php:16 MediaWikiNoBaseException Instantiating /Exception directly is not allowed. Use SPL exceptions if the exception is unchecked, or define a custom exception class otherwise. See https://w.wiki/6nur

tests/plugins/NoBaseExceptionPlugin/basic/test.php

@@ -0,0 +1,30 @@
+<?php
+
+function testGood() {
+	$x = new RuntimeException();
+	throw new RuntimeException();
+	$class = RuntimeException::class;
+	throw new $class;
+	$randClass = rand() ? RuntimeException::class : LogicException::class;
+	throw new $randClass;
+	$potentiallyAnyException = getSomeException();
+	throw new $potentiallyAnyException;
+}
+
+function testBad() {
+	$x = new Exception();
+	throw new Exception();
+}
+
+function testIdeallyBad() {
+	// Things that should trigger a warning, but aren't handled yet.
+	$class = Exception::class;
+	throw new $class;
+	$randClass = rand() ? RuntimeException::class : Exception::class;
+	throw new $randClass;
+}
+
+function getSomeException(): Exception {
+	return $GLOBALS['this-could-be-a-subclass'];
+}
+

tests/plugins/NoBaseExceptionPlugin/config.php

@@ -0,0 +1,17 @@
+<?php
+
+use Phan\Config;
+
+$baseCfg = require __DIR__ . '/../base-plugin-test-config.php';
+
+$baseCfg['plugins'] = [
+	Config::projectPath( __DIR__ . '/../../../src/Plugin/NoBaseExceptionPlugin.php' )
+];
+
+$baseCfg['whitelist_issue_types'] = [
+	// Fun fact: we can't use NoBaseExceptionPlugin::ISSUE_TYPE as that would trigger the composer autoloader
+	// and cause a PHP fatal error due to duplicated class definition when phan tries to `require` the plugin file.
+	'MediaWikiNoBaseException',
+];
+
+return $baseCfg;

tests/plugins/NoBaseExceptionPlugin/homonym/expectedResults.txt

@@ -0,0 +1 @@
+plugins/NoBaseExceptionPlugin/homonym/test.php:13 MediaWikiNoBaseException Instantiating /Exception directly is not allowed. Use SPL exceptions if the exception is unchecked, or define a custom exception class otherwise. See https://w.wiki/6nur

tests/plugins/NoBaseExceptionPlugin/homonym/test.php

@@ -0,0 +1,15 @@
+<?php
+
+namespace NoBaseExceptionPluginTestHomonym {
+	class Exception extends \Exception {
+
+	}
+
+	function testGood() {
+		throw new Exception();
+	}
+
+	function testBad() {
+		throw new \Exception();
+	}
+}

tests/plugins/NoBaseExceptionPlugin/parent/test.php

@@ -0,0 +1,8 @@
+<?php
+
+class MyException extends Exception {
+	public function __construct() {
+		// Calling the parent constructor from Exception is fine.
+		parent::__construct( 'Some predefined message' );
+	}
+}