[Followup] Fix Drupal.Commenting.FunctionComment.MissingReturnType returns for NULL

Decide if we want to change this

If we ever want to use proper typehints for function/method parameters and return value, and declare(strict_types = 1);, then the answer is yes.

<?php

/**
 * @param bool $string
 *
 * @return string|null
 */
function my_nullable_string_phpdoc(bool $string) {
  if ($string) {
    return 'okay';
  }
}

function my_nullable_string_typehint_fail(bool $string): ?string {
  if ($string) {
    return 'okay';
  }
}

function my_nullable_string_typehint_ok(bool $string): ?string {
  if ($string) {
    return 'okay';
  }

  return NULL;
}

my_nullable_string_phpdoc(TRUE);
my_nullable_string_phpdoc(FALSE);

my_nullable_string_typehint_fail(TRUE);
my_nullable_string_typehint_fail(FALSE);

my_nullable_string_typehint_ok(TRUE);
my_nullable_string_typehint_ok(FALSE);

declare(strict_types=1); doesn't affect this behaviour.

PhpStorm reported 428 occurrences of this kind of error (not explicitly defined return value).
In the commit 2022-09-27 57b569d I fixed only very few of them.

Several of them would be straightforward as well, like the diff between the
my_nullable_string_typehint_fail
and
my_nullable_string_typehint_ok
see comment #4

But there are quite a few tricky ones.

\Drupal\Core\Template\AttributeValueBase::render returns string only in a certain condition, otherwise returns null – not explicitly, with typehint it would throw a "Fatal error: Uncaught TypeError" –, but according to the PHPDoc it always returns a string.
There are several function/method like this.
For example: \hook_help(). Even the example code is "wrong", so all the implementations.
No to mention that \action_help() returns a \Drupal\Core\StringTranslation\TranslatableMarkup instance.

One more example: \Drupal\views\Plugin\views\field\EntityField::getFieldStorageDefinition

\Drupal\locale\PluralFormula::loadFormulae
PHPDoc @return array, but the actual return value is void.
This one also detected by PHPCS Drupal.Commenting.FunctionComment.InvalidNoReturn.

I disagree that we need to change the returns to satisfy the type here, I think it's perfectly valid to have return; but the typehint in that case should be @return string|void, so it's a string or nothing, null is not the same as void/nothing.

@borisson_ There is no such a thing in PHP: string|void

https://wiki.php.net/rfc/union_types_v2#supported_types

The void type can never be part of a union. As such, types like T|void are illegal in all positions, including return types.

The void type indicates that the function has no return value, and enforces that argument-less return; is used to return from the function. It is fundamentally incompatible with non-void return types.

What is likely intended instead is ?T, which allows returning either T or null.

I shared this in Slack but never got around to posting to this thread. With regards to whether we should or should not enforce a final return NULL;, I believe we should.

I think we should just follow the wider PHP community and be explicit in our final NULL returns. PHPStan already warns about this exact scenario.

https://phpstan.org/r/848a5b50-4525-415e-a7aa-cadfea465095

Even if I follow the initial hint of "remove NULL because it never returns NULL" then I get "Function test() should return string but return statement is missing." which also indicates that I forgot to think about something.

https://phpstan.org/r/9a0087a9-05ae-47d9-bc25-da93dc1f7cae

Adding a return NULL at the end indicates that I've thought about my return types and the code is complete. If this should never return NULL then apparently $b not being always TRUE is an error and I should end with throwing a helpful exception instead.

@Kingdutch : Thanks for the comment. :-)

I started to work on this, but I have concerns:

1. The diff will be quiet large, and reviewer/maintainer will ask for to split into smaller issues.
2. There are lot of controversial code and PhpDoc.

For example:

1. PhpDoc \Drupal\Component\Plugin\Mapper\MapperInterface::getInstance
2. Implementation \Drupal\Core\Archiver\ArchiverManager::getInstance

By adding return NULL; won't change that how the code behaves, and it is still won't match to the PhpDoc.
Is it part of the scope to change @return object|false to @return object|null?

I think yes, It would be good to clarify in advance.

Other question is how to solve the simple ones.

Original:

  public function adminSummary() {
    if (!empty($this->options['exposed'])) {
      return $this->t('exposed');
    }
  }

Style 1

  public function adminSummary() {
    if (!empty($this->options['exposed'])) {
      return $this->t('exposed');
    }

    return NULL;
  }

Style 2

  public function adminSummary() {
    return !empty($this->options['exposed']) ?
      $this->t('exposed')
      : NULL;
  }

Can the MR be updated for 11.x please.

11.x is the current development branch, so code has to be committed there first to be backported.

So searching for " but return statement is missing" there still appear to be 89 instances in phpstan-baseline. Were those left out specifically?

Give them a closer look.

For example this one: \Drupal\Core\Database\Query\Delete::execute

Think it needs to be documented why we are skipping these 89 instances

Some functions are

save(),
getFormId(),
validateForm(),
etc

If the inherited docs say it returns a value and we are leaving in the phpstan file think it needs to be stated why.

I closed #3561384: ModerationInformationInterface::getDefaultRevisionId missing typing as a duplicate of this one.