ReturnAssignmentFixer needs to also remove inline docblock for the variable it's removing

[dmvdbrugge]

I was unsure whether to report this as priority issue where return_assignment needs to run before phpdoc_to_comment or if it's the responsibility of ReturnAssignmentFixer.

it's not a prio issue, comment shall be removed
-- @keradus on gitter

The PHP version you are using ($ php -v):

PHP 7.1.18 (cli) (built: May 25 2018 19:18:59) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.1.18, Copyright (c) 1999-2018, by Zend Technologies

PHP CS Fixer version you are using ($ php-cs-fixer -V):

PHP CS Fixer 2.12.1 Long Journey by Fabien Potencier and Dariusz Ruminski

The command you use to run PHP CS Fixer:

vendor/bin/php-cs-fixer fix -v --using-cache=no

The configuration file you are using, if any:

Basically the same as dmvdbrugge/dynamic-components, except explicit_string_variable, header_comment, no_alternative_syntax, and no_unset_on_property

If applicable, please provide minimum samples of PHP code (as plain text, not screenshots):

• before running PHP CS Fixer (no changes):

function return_assignment()
{
    /** @var string[] $intermediate */
    $intermediate = ['test'];

    return $intermediate;
}

• after first run:

--- Original
+++ New
@@ @@
 function return_assignment()
 {
     /** @var string[] $intermediate */
-    $intermediate = ['test'];
-
-    return $intermediate;
+    return ['test'];
 }

• after second run:

--- Original
+++ New
@@ @@
 function return_assignment()
 {
-    /** @var string[] $intermediate */
+    /* @var string[] $intermediate */
     return ['test'];
 }

• expected:
  Either immediately arrive at the last step, or docblock should've been removed.

Meta: this is case 14 of #3844

[SpacePossum]

Thanks for the report, it was briefly discussed on gitter.
I think the issue is valid, but we need to check how to resolve.
Meanwhile this issue is a duplicate of the WIP/POC PR #3806 , therefore I'm closing it (so not to stop it from happening or denying this is an issue)

[dmvdbrugge]

As the mentioned PR got closed without resolving anything, can this issue be reopened?

[kubawerlos]

So what is the plan? I cannot see what else could we do other than removing all non-meaningful tokens between the removed assignment and return.

@keradus did you said "comment shall be removed" in this particular case?

[SpacePossum]

I think it is best to have a fixer that changes invalid PHPDocs into comments. If that means that in some cases a human must check the comments because we cannot process free text, than that is fine by me :)

[dmvdbrugge]

I think it is best to have a fixer that changes invalid PHPDocs into comments

That fixer exists (PhpdocToCommentFixer), and is in fact how I found this issue, because if you go that way, that fixer should run after ReturnAssignmentFixer (which it currently doesn't).

Upon finding the issue, I went on gitter, explained the situation, and got answered by @keradus. And that's why the opening lines of this issue say:

I was unsure whether to report this as priority issue where return_assignment needs to run before phpdoc_to_comment or if it's the responsibility of ReturnAssignmentFixer.

it's not a prio issue, comment shall be removed
-- @keradus on gitter

The simplest solution could be along the lines of (pseudo-code incoming):

$prev = $tokenOfVariableToBeRemoved->findPreviousNonWhitespaceToken();
if ($prev->isGivenKind(T_DOC_COMMENT)) {
    if ($line = Annotation(prev)->contains($tokenOfVariableToBeRemoved->content)) {
        $line->remove();
    }
}

Which should then run before NoEmptyPhpdocFixer. This does only cover the "simple" case as described in the issue, but I think that would be the most common case anyway.

I would also be fine with just the priority change, but that's not what @keradus told me to go for.

[kubawerlos]

I would also be fine with just the priority change

Me too. The problem is ReturnAssignmentFixer has a priority -15 and PhpdocToCommentFixer has 25 - of course, both for a reason. The change would affect many other fixers as well.

[SpacePossum]

Kindly note that I think @keradus and I have different opinions on this topic.
I think the fixer should not remove the doc and the leftover doc should be handled by another fixer; either changed to a comment or be removed, or maybe no action is needed on the issue at all.
Before doing work on it maybe we should wrap up the discussion first :)

[enumag]

I didn't read the whole discussion but I disagree with the title. With docblock types that include generics the docblock might be a necessity for PHPStan or Psalm to not report type mismatch errors. Therefore rather than removing the docblock as well I think a variable with a docblock should NOT be removed by this fixer.

[SpacePossum]

Hi @enumag ,

I think you request another feature than in the original report indeed.
So:

• remove (or update) the PHPDoc when removing the variable (original request)
• do not remove the variable (nor the PHPDoc) if the variable has a PHPDoc with type hint information (your request)

[enumag]

I see. Should I create a separate issue then?

[SpacePossum]

I would recommend that :)

[Wirone]

I'm going to close this because issue mentions Fixer version which is not maintained anymore. Feel free to reproduce it on latest version (if problem still exists) and create new issue.