Fix false positive/negative PhanTypeMissingReturn instances

The check was wrong and should have been checking for never returning.
Return statements can be omitted if a function unconditionally exits.

Closes phan#4537

.phan/plugins/AddNeverReturnTypePlugin.php

@@ -13,7 +13,7 @@
 use Phan\PluginV3\AnalyzeMethodCapability;
 
 /**
- * This file checks if a function, closure or method will not return (and has no overrides).
+ * This plugin checks if a function or method will not return (and has no overrides).
  * If the function doesn't have a return type of never.
  * then this plugin will emit an issue.
  *
@@ -67,20 +67,22 @@ public function analyzeMethod(
             return;
         }
 
-        if (!BlockExitStatusChecker::willUnconditionallyNeverReturn($stmts_list)) {
-            return;
-        }
         if ($method->getUnionType()->hasType(NeverType::instance(false))) {
             return;
         }
         if ($method->isOverriddenByAnother()) {
             return;
         }
+
+        // This modifies the nodes in place, check this last
+        if (!BlockExitStatusChecker::willUnconditionallyNeverReturn($stmts_list)) {
+            return;
+        }
         self::emitIssue(
             $code_base,
             $method->getContext(),
             'PhanPluginNeverReturnMethod',
-            "Function {FUNCTION} never returns and has a return type of {TYPE}, but phpdoc type {TYPE} could be used instead",
+            "Method {METHOD} never returns and has a return type of {TYPE}, but phpdoc type {TYPE} could be used instead",
             [$method->getRepresentationForIssue(), $method->getUnionType(), 'never']
         );
     }
@@ -104,10 +106,11 @@ public function analyzeFunction(
             return;
         }
 
-        if (!BlockExitStatusChecker::willUnconditionallyNeverReturn($stmts_list)) {
+        if ($function->getUnionType()->hasType(NeverType::instance(false))) {
             return;
         }
-        if ($function->getUnionType()->hasType(NeverType::instance(false))) {
+        // This modifies the nodes in place, check this last
+        if (!BlockExitStatusChecker::willUnconditionallyNeverReturn($stmts_list)) {
             return;
         }
         self::emitIssue(

.phan/plugins/README.md

@@ -596,6 +596,16 @@ This is only useful in applications or libraries that print output in only a few
 
 Suppression comments can use the issue name `PhanPluginRemoveDebugAny` to suppress all issue types emitted by this plugin.
 
+#### AddNeverReturnTypePlugin.php
+
+This plugin checks if a function or method will not return (and has no overrides).
+If the function doesn't have a return type of never.
+then this plugin will emit an issue.
+Closures and short error functions are currently not checked
+
+- **PhanPluginNeverReturnMethod**: `Method {METHOD} never returns and has a return type of {TYPE}, but phpdoc type {TYPE} could be used instead`
+- **PhanPluginNeverReturnFunction**: `Function {FUNCTION} never returns and has a return type of {TYPE}, but phpdoc type {TYPE} could be used instead`
+
 ### 4. Demo plugins:
 
 These files demonstrate plugins for Phan.

NEWS.md

@@ -17,6 +17,12 @@ Plugins:
 Bug fixes:
 - Fix type inference logic that was looking for array specializations rather than array or any array subtype (#4512)
 - Fix false positive `PhanUnreferencedClosure`/`PhanUnreferencedFunction` seen when a closure/function name was passed to a function such as `uasort` that already had a plugin analyzing calls of the closure. (#4090, #4519)
+- Fix false positive/negative `PhanTypeMissingReturn*` instances. (#4537)
+
+  The check was wrong and should have been checking for a statement list that throws/exits.
+  Return statements can be omitted if a function unconditionally exits.
+
+  Also, check for the real `never` return type when emitting issues
 
 Maintenance:
 - Fix old return type signature for `get_headers` (#3273)

src/Phan/Analysis/BlockExitStatusChecker.php

@@ -816,7 +816,7 @@ public static function willUnconditionallyThrowOrReturn(Node $node): bool
     }
 
     /**
-     * Will the node $node unconditionally throw or exit
+     * Will the node $node unconditionally throw or exit (or infinitely loop)
      */
     public static function willUnconditionallyNeverReturn(Node $node): bool
     {

src/Phan/Analysis/PostOrderAnalysisVisitor.php

@@ -1498,10 +1498,9 @@ public function visitClosure(Node $node): Context
 
         if (!$return_type->isEmpty()
             && !$func->hasReturn()
-            && !self::declOnlyThrows($node)
-            && !$return_type->hasType(VoidType::instance(false))
-            && !$return_type->hasType(NullType::instance(false))
-            && !$return_type->hasType(NeverType::instance(false))
+            && !self::declNeverReturns($node)
+            && !$return_type->isNull()
+            && !$return_type->isNeverType()
         ) {
             $this->warnTypeMissingReturn($func, $node);
         }
@@ -3022,9 +3021,10 @@ public function visitMethod(Node $node): Context
             && !$has_interface_class
             && !$return_type->isEmpty()
             && !$method->hasReturn()
-            && !self::declOnlyThrows($node)
+            && !self::declNeverReturns($node)
             && !$return_type->hasType(VoidType::instance(false))
             && !$return_type->hasType(NullType::instance(false))
+            && !$return_type->hasType(NeverType::instance(false))
         ) {
             $this->warnTypeMissingReturn($method, $node);
         }
@@ -3084,7 +3084,7 @@ public function visitFuncDecl(Node $node): Context
 
         if (!$return_type->isEmpty()
             && !$function->hasReturn()
-            && !self::declOnlyThrows($node)
+            && !self::declNeverReturns($node)
             && !$return_type->hasType(VoidType::instance(false))
             && !$return_type->hasType(NullType::instance(false))
         ) {
@@ -5039,12 +5039,12 @@ private function warnBreakOrContinueWithoutLoop(Node $node): void
      * @return bool
      * True when the decl can only throw an exception or return or exit()
      */
-    private static function declOnlyThrows(Node $node): bool
+    private static function declNeverReturns(Node $node): bool
     {
         // Work around fallback parser generating methods without statements list.
         // Otherwise, 'stmts' would always be a Node due to preconditions.
         $stmts_node = $node->children['stmts'];
-        return $stmts_node instanceof Node && BlockExitStatusChecker::willUnconditionallyThrowOrReturn($stmts_node);
+        return $stmts_node instanceof Node && BlockExitStatusChecker::willUnconditionallyNeverReturn($stmts_node);
     }
 
     /**

tests/files/expected/0242_void_71.php.expected

@@ -4,3 +4,4 @@
 %s:5 PhanRedefineFunction Function f3 defined at %s:5 was previously defined at %s:4
 %s:5 PhanTypeMissingReturnReal Method \f3 is declared to return int in its real type signature but has no return value
 %s:6 PhanSyntaxReturnExpectedValue Syntax error: Function f5() with return type ?string must return a value (did you mean "return null" instead of "return"?)
+%s:6 PhanTypeMissingReturnReal Method \f5 is declared to return ?string in its real type signature but has no return value

tests/files/expected/0279_void_return.php.expected

@@ -2,4 +2,5 @@
 %s:9 PhanSyntaxReturnExpectedValue Syntax error: Function f() with return type \DateTime must return a value (did you mean "return null" instead of "return"?)
 %s:10 PhanRedundantCondition Redundant attempt to cast true of type true to truthy
 %s:10 PhanTypeMismatchReturn Returning unknownType() of type 42 but f() is declared to return \DateTime
+%s:14 PhanTypeMissingReturnReal Method \g is declared to return int in its real type signature but has no return value
 %s:15 PhanSyntaxReturnExpectedValue Syntax error: Function g() with return type int must return a value (did you mean "return null" instead of "return"?)

tests/php80_files/expected/009_mixed_error.php.expected

@@ -1,5 +1,6 @@
 %s:2 PhanUnreferencedClass Possibly zero references to class \Base9
 %s:3 PhanPluginUseReturnValueNoopVoid The function/method \Base9::test() is declared to return mixed and it has no side effects
+%s:3 PhanTypeMissingReturnReal Method \Base9::test is declared to return mixed in its real type signature but has no return value
 %s:3 PhanUnreferencedPublicMethod Possibly zero references to public method \Base9::test()
 %s:4 PhanSyntaxReturnExpectedValue Syntax error: Function test() with return type mixed must return a value (did you mean "return null" instead of "return"?)
 %s:9 PhanUnreferencedPublicMethod Possibly zero references to public method \Base9B::test()

tests/plugin_test/expected/043_throws.php.expected

@@ -1,5 +1,5 @@
 src/043_throws.php:6 PhanPluginNeverReturnFunction Function \throwOutOfBoundsException() never returns and has a return type of void, but phpdoc type never could be used instead
-src/043_throws.php:14 PhanPluginNeverReturnMethod Function \ExampleThrow43::throwInvalidArgumentException() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/043_throws.php:14 PhanPluginNeverReturnMethod Method \ExampleThrow43::throwInvalidArgumentException() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
 src/043_throws.php:20 PhanThrowTypeAbsent \ExampleThrow43::example() can throw throwOutOfBoundsException of type \OutOfBoundsException here, but has no '@throws' declarations for that class
 src/043_throws.php:22 PhanThrowTypeAbsentForCall \ExampleThrow43::example() can throw \InvalidArgumentException because it calls \ExampleThrow43::throwInvalidArgumentException(), but has no '@throws' declarations for that class
 src/043_throws.php:30 PhanThrowTypeAbsentForCall \ExampleThrow43::example2() can throw \InvalidArgumentException because it calls \ExampleThrow43::throwInvalidArgumentException(), but has no '@throws' declarations for that class

tests/plugin_test/expected/085_throw_type_mismatch.php.expected

@@ -4,4 +4,4 @@ src/085_throw_type_mismatch.php:19 PhanTypeInvalidThrowStatementNonThrowable \NS
 src/085_throw_type_mismatch.php:21 PhanThrowTypeMismatch \NS626\C::main() throws new FinalClass85() of type \NS626\FinalClass85 here, but it only has declarations of '@throws \InvalidArgumentException'
 src/085_throw_type_mismatch.php:21 PhanTypeInvalidThrowStatementNonThrowable \NS626\C::main() can throw new FinalClass85() of type \NS626\FinalClass85 here which can't cast to \Throwable
 src/085_throw_type_mismatch.php:23 PhanThrowTypeMismatchForCall \NS626\C::main() throws \TypeError because it calls \NS626\C::throw(), but it only has declarations of '@throws \InvalidArgumentException'
-src/085_throw_type_mismatch.php:29 PhanPluginNeverReturnMethod Function \NS626\C::throw() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/085_throw_type_mismatch.php:29 PhanPluginNeverReturnMethod Method \NS626\C::throw() never returns and has a return type of (empty union type), but phpdoc type never could be used instead

tests/plugin_test/expected/123_throw_static.php.expected

@@ -1,9 +1,9 @@
 src/123_throw_static.php:2 PhanUnreferencedClass Possibly zero references to class \ThrowingException688
-src/123_throw_static.php:6 PhanPluginNeverReturnMethod Function \ThrowingException688::throw() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/123_throw_static.php:6 PhanPluginNeverReturnMethod Method \ThrowingException688::throw() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
 src/123_throw_static.php:6 PhanUnreferencedPublicMethod Possibly zero references to public method \ThrowingException688::throw()
-src/123_throw_static.php:13 PhanPluginNeverReturnMethod Function \ThrowingException688::throwOther() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/123_throw_static.php:13 PhanPluginNeverReturnMethod Method \ThrowingException688::throwOther() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
 src/123_throw_static.php:13 PhanUnreferencedPublicMethod Possibly zero references to public method \ThrowingException688::throwOther()
 src/123_throw_static.php:14 PhanThrowTypeMismatch \ThrowingException688::throwOther() throws new RuntimeException('unexpected') of type \RuntimeException here, but it only has declarations of '@throws static'
-src/123_throw_static.php:20 PhanPluginNeverReturnMethod Function \ThrowingException688::throwStatic() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/123_throw_static.php:20 PhanPluginNeverReturnMethod Method \ThrowingException688::throwStatic() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
 src/123_throw_static.php:20 PhanUnreferencedPublicMethod Possibly zero references to public method \ThrowingException688::throwStatic()
 src/123_throw_static.php:21 PhanThrowTypeMismatch \ThrowingException688::throwStatic() throws new static('unexpected') of type static here, but it only has declarations of '@throws \RuntimeException'

tests/plugin_test/expected/133_throw_in_to_string.php.expected

@@ -1,3 +1,3 @@
-src/133_throw_in_to_string.php:6 PhanPluginNeverReturnMethod Function \X133::__toString() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
+src/133_throw_in_to_string.php:6 PhanPluginNeverReturnMethod Method \X133::__toString() never returns and has a return type of (empty union type), but phpdoc type never could be used instead
 src/133_throw_in_to_string.php:6 PhanThrowCommentInToString \X133::__toString() documents that it throws \RuntimeException, but throwing in __toString() is a fatal error prior to PHP 7.4
 src/133_throw_in_to_string.php:7 PhanThrowStatementInToString \X133::__toString() throws \RuntimeException here, but throwing in __toString() is a fatal error prior to PHP 7.4

tests/plugin_test/expected/202_never_return.php.expected

@@ -0,0 +1 @@
+src/202_never_return.php:7 PhanUnusedProtectedMethodParameter Parameter $x is never used

tests/plugin_test/src/202_never_return.php

@@ -0,0 +1,25 @@
+<?php
+
+class BaseClass {
+    /**
+     * @return never
+     */
+    protected function fatalError( $x ) {
+        exit();
+    }
+}
+
+class ChildClass extends BaseClass {
+    /**
+     * @return never
+     */
+    public function fatalError( $x ) {
+        echo implode(['a' => $x]);
+        parent::fatalError( $x );
+    }
+
+    public function main() {
+        $this->fatalError( 42 );
+    }
+}
+var_dump((new ChildClass())->main());